[PATCHish] IfConversion; lost edges for some diamonds

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?

if-try.patch (678 Bytes)

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

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

An actual test case would be useful, as I would much prefer to commit this change with a test case. Hexagon tends to have a lot of non-analyzable branches, you may be able to create a test case for your target and compile it for Hexagon to reproduce.

I’m pretty sure the code is incorrect, but It hasn’t been a priority because there aren’t currently any failures on upstream targets.

I’ll take a look at triangles as well.