[llvm] r219223 - Two case switch to select optimization

[Hal Finkel]

----- Original Message -----
> From: "Marcello Maggioni" <hayarms at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Sunday, October 12, 2014 8:15:32 PM
> Subject: Re: [llvm] r219223 - Two case switch to select optimization
> 
> 
> 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.

That's correct.

I'm not sure how good the LangRef is in detailing this. If this is not spelled out, we should likely improve it. I've ended up fixing a bunch of code over the years because it did not handle this correctly.

 -Hal

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

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory