[llvm] r219223 - Two case switch to select optimization

[Marcello Maggioni]

I see, thanks for the specification Hal!

I was confused because the verifier tests it in this way:

      Assert1(PN->getNumIncomingValues() == Preds.size(),
              "PHINode should have one entry for each predecessor of its "
              "parent basic block!", PN);

where Preds are the predecessors of the block. Because
getNumIncomingValues() returns the number of operands, which should be
equal to the number of incoming edges , I thought that if multiple edges
are specified for the same block this check would fail.
Apparently then also the predecessors are stored multiple times.

Marcello

2014-10-12 18:06 GMT-07:00 Hal Finkel <hfinkel at anl.gov>:

> Marcello,
>
> Multiple incoming edges are allowed, so long as the corresponding value is
> the same. Different values from the same predecessor are not permitted. If
> there is some code that does not handle this correctly, it will need to be
> fixed.
>
>  -Hal
>
> ----- Original Message -----
> > From: "Marcello Maggioni" <hayarms at gmail.com>
> > To: "Marcello Maggioni" <hayarms at gmail.com>, llvm-commits at cs.uiuc.edu
> > Sent: Sunday, October 12, 2014 6:44:54 PM
> > Subject: Re: [llvm] r219223 - Two case switch to select optimization
> >
> >
> >
> > I did a fast research into the LLVM base code and seems like, even if
> > having multiple incoming edges for the same block is forbidden by
> > the verifier through a check at line Verifier.cpp:1146, it is a
> > common place assumption in SimplifyCFG that if a block branches to
> > the same target multiple times the PHI should have multiple incoming
> > edges from the same block.
> >
> >
> > I find this pretty strange ... I tried to remove the source of double
> > incoming edges for the specific case in the example posted by Joerg,
> > but doing so breaks other code that EXPECTS multiple incoming edges
> > for the same block in certain conditions (like a conditional branch
> > targeting the same block two times with both the True and False
> > branches).
> >
> >
> > If this is the direction we want to go (continuing to support
> > multiple incoming edges from the same block) tomorrow I will post a
> > patch for switch to select that takes into consideration this when
> > multiple cases of a switch target the same block and I will add Hans
> > and Jin as reviewers.
> >
> >
> > Cheers,
> > Marcello
> >
> >
> >
> >
> >
> > 2014-10-12 14:40 GMT-07:00 Marcello Maggioni < hayarms at gmail.com > :
> >
> >
> >
> > Hmm,
> >
> >
> > compiling your test I've noticed that even before entering the
> > SwitchToSelect optimization the PHI that breaks the module already
> > has two incoming edges for the same predecessor:
> >
> >
> > This is the IR just entering SwitchToSelect:
> >
> >
> >
> >
> > ; Function Attrs: nounwind ssp uwtable
> >
> > define i32 @fn1() #0 {
> >
> > entry:
> >
> > %0 = load i32* @b, align 4
> >
> > %tobool = icmp eq i32 %0, 0
> >
> > br i1 %tobool, label %if.end3, label %if.then
> >
> >
> >
> >
> > if.then: ; preds = %entry
> >
> > %1 = load i32* @a, align 4
> >
> > switch i32 %1, label %if.end3 [
> >
> > i32 5, label %return
> >
> > i32 0, label %return
> >
> > ]
> >
> >
> >
> >
> > if.end3: ; preds = %if.then, %entry
> >
> > br label %return
> >
> >
> >
> >
> > return: ; preds = %if.then, %if.then, %if.end3
> >
> > %retval.0 = phi i32 [ 0, %if.end3 ], [ %1, %if.then ], [ %1, %if.then
> > ]
> >
> > ret i32 %retval.0
> >
> > }
> >
> >
> > Is this condition expected to be handled by the switch lowering? Or
> > is the switch formation that probably introduced this malformed?
> > Is pretty easily to workaround this in SwitchToSelect to reiterate
> > Incoming Edge deletion when the switch is removed until no more
> > edges from the same predecessor are found, but I guess if we have
> > some other issue more up in the pipeline.
> >
> >
> > 2014-10-12 14:35 GMT-07:00 Marcello Maggioni < hayarms at gmail.com > :
> >
> >
> >
> >
> >
> > Thanks again Joerg, I missed the attachment!
> >
> >
> > Marcello
> >
> >
> > 2014-10-12 13:50 GMT-07:00 Joerg Sonnenberger <
> > joerg at britannica.bec.de > :
> >
> >
> >
> >
> > On Sun, Oct 12, 2014 at 01:09:25PM -0700, Marcello Maggioni wrote:
> > > Thanks Joerg for reverting and thanks Jiangning for reporting the
> > > problem.
> > >
> > > Would it be possible to have the snippet of the code that is
> > > causing the
> > > issue or is it not publicly available? In order to be able to debug
> > > this
> > > more effectively and safely.
> >
> > See my earlier post in this thread for a test case.
> >
> > Joerg
> >
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141012/912560b2/attachment.html>