llvm dev - Jan 2017 - [PATCHish] IfConversion; lost edges for some diamonds

On Tue, Jan 10, 2017 at 2:31 AM, Peter A Jonsson <pj at sics.se> wrote:
> Hi Kyle,
>
> my apologies for mailing you directly but it seems new user creation is
> disabled on the llvm bugzilla.
>
> We sometime lose edges during IfConversion of diamonds and it’s not
> obvious how to reproduce on an upstream target. The documentation for
> HasFallThrough says *may* fallthrough which I interpret as “this will be
> true whenever we aren’t sure” but IfConverter::AnalyzeBranches() contains
> the code:
>
> BBI.HasFallThrough = BBI.IsBrAnalyzable && BBI.FalseBB == nullptr;
>
BBI.HasFallThrough really means "Has Analyzable fallthrough." So this
line
is correct.

>
> So HasFallThrough is false whenever IsBrAnalyzable is false. That is
> common because IsBrAnalyzable is false when analyzeBranch() is true (its
> default implementation). Is it possible that HasFallThrough should also be
> true when IsBrAnalyzable is false? In other words, instead initialized as:
>
> BBI.HasFallThrough = !BBI.IsBrAnalyzable || BBI.FalseBB == nullptr;
>
> ? That change makes my test pass so it would be wonderfully simple if that
> is all that it takes.

I suspect there is a simple change, but you're looking in the wrong place.
Can you let me know if the attached patch works for you?

>
> Best Wishes,
>
> Peter
>
> —
> Peter A. Jonsson, Ph.D.
> Swedish Institute of Computer Science
> Box 1263, SE-164 29 Kista, Sweden
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170110/37432f16/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: if-try.patch
Type: text/x-patch
Size: 678 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170110/37432f16/attachment.bin>

Works like a charm, thank you!

I was wrong about the intention of HasFallThrough but your patch fixes the same
thing I was going for. Can the documentation for HasFallThrough be updated so
that it’s clear that HasFallThrough is certain and the “may” applies to whether
the fall through will happen or not?

Best Wishes,

Peter
> On 10 Jan 2017, at 21:22, Kyle Butt <iteratee at google.com> wrote:
> 
> 
> 
> On Tue, Jan 10, 2017 at 2:31 AM, Peter A Jonsson <pj at sics.se>
wrote:
> Hi Kyle,
> 
> my apologies for mailing you directly but it seems new user creation is
disabled on the llvm bugzilla.
> 
> We sometime lose edges during IfConversion of diamonds and it’s not obvious
how to reproduce on an upstream target. The documentation for HasFallThrough
says *may* fallthrough which I interpret as “this will be true whenever we
aren’t sure” but IfConverter::AnalyzeBranches() contains the code:
> 
> BBI.HasFallThrough = BBI.IsBrAnalyzable && BBI.FalseBB == nullptr;
> 
> BBI.HasFallThrough really means "Has Analyzable fallthrough." So
this line is correct.
>  
> 
> So HasFallThrough is false whenever IsBrAnalyzable is false. That is common
because IsBrAnalyzable is false when analyzeBranch() is true (its default
implementation). Is it possible that HasFallThrough should also be true when
IsBrAnalyzable is false? In other words, instead initialized as:
> 
> BBI.HasFallThrough = !BBI.IsBrAnalyzable || BBI.FalseBB == nullptr;
> 
> ? That change makes my test pass so it would be wonderfully simple if that
is all that it takes.
> 
> I suspect there is a simple change, but you're looking in the wrong
place. Can you let me know if the attached patch works for you?
>  
> 
> Best Wishes,
> 
> Peter
> 
> —
> Peter A. Jonsson, Ph.D.
> Swedish Institute of Computer Science
> Box 1263, SE-164 29 Kista, Sweden
> 
> <if-try.patch>

Hi Kyle,

a colleague found what looks like a similar problem in
IfConvert::IfConvertTriangle(). Is it possible that your fix is needed in
IfConvert::IfConvertTriangle() too? I read the comment as “we can only merge
blocks if we are guaranteed there is no fallthrough” which you said isn’t the
intended meaning of HasFallThrough.

I’m also a bit unsure of the direction where things are heading: are you
planning to merge your patch or is this a non-issue for upstream-targets?

Best Wishes,

Peter
> On 13 Jan 2017, at 10:39, Peter A Jonsson <pj at sics.se> wrote:
> 
> Works like a charm, thank you!
> 
> I was wrong about the intention of HasFallThrough but your patch fixes the
same thing I was going for. Can the documentation for HasFallThrough be updated
so that it’s clear that HasFallThrough is certain and the “may” applies to
whether the fall through will happen or not?
> 
> Best Wishes,
> 
> Peter
> 
>> On 10 Jan 2017, at 21:22, Kyle Butt <iteratee at google.com>
wrote:
>> 
>> 
>> 
>> On Tue, Jan 10, 2017 at 2:31 AM, Peter A Jonsson <pj at sics.se>
wrote:
>> Hi Kyle,
>> 
>> my apologies for mailing you directly but it seems new user creation is
disabled on the llvm bugzilla.
>> 
>> We sometime lose edges during IfConversion of diamonds and it’s not
obvious how to reproduce on an upstream target. The documentation for
HasFallThrough says *may* fallthrough which I interpret as “this will be true
whenever we aren’t sure” but IfConverter::AnalyzeBranches() contains the code:
>> 
>> BBI.HasFallThrough = BBI.IsBrAnalyzable && BBI.FalseBB ==
nullptr;
>> 
>> BBI.HasFallThrough really means "Has Analyzable fallthrough."
So this line is correct.
>> 
>> 
>> So HasFallThrough is false whenever IsBrAnalyzable is false. That is
common because IsBrAnalyzable is false when analyzeBranch() is true (its default
implementation). Is it possible that HasFallThrough should also be true when
IsBrAnalyzable is false? In other words, instead initialized as:
>> 
>> BBI.HasFallThrough = !BBI.IsBrAnalyzable || BBI.FalseBB == nullptr;
>> 
>> ? That change makes my test pass so it would be wonderfully simple if
that is all that it takes.
>> 
>> I suspect there is a simple change, but you're looking in the wrong
place. Can you let me know if the attached patch works for you?
>> 
>> 
>> Best Wishes,
>> 
>> Peter
>> 
>> —
>> Peter A. Jonsson, Ph.D.
>> Swedish Institute of Computer Science
>> Box 1263, SE-164 29 Kista, Sweden
>> 
>> <if-try.patch>
>