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

[Peter A Jonsson via llvm-dev]

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>
>