[llvm-dev] [PATCHish] IfConversion; lost edges for some diamonds
Kyle Butt via llvm-dev
llvm-dev at lists.llvm.org
Tue Jan 24 16:00:27 PST 2017
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.
On Tue, Jan 17, 2017 at 10:44 PM, Peter A Jonsson <pj at sics.se> wrote:
> 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,
> > 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>
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-dev