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

[Kyle Butt via llvm-dev]

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,
>
> 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>
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170124/d92e4b16/attachment.html>