[llvm-commits] [PATCH] Phi speculation improvement for BasicAA

[Hal Finkel]

----- Original Message -----
> From: "Eli Friedman" <eli.friedman at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Friday, November 16, 2012 8:03:44 PM
> Subject: Re: [llvm-commits] [PATCH] Phi speculation improvement for BasicAA
> 
> On Fri, Nov 16, 2012 at 4:57 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > ----- Original Message -----
> >> From: "Eli Friedman" <eli.friedman at gmail.com>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: "Commit Messages and Patches for LLVM"
> >> <llvm-commits at cs.uiuc.edu>
> >> Sent: Friday, November 16, 2012 6:30:49 PM
> >> Subject: Re: [llvm-commits] [PATCH] Phi speculation improvement
> >> for BasicAA
> >>
> >> On Fri, Nov 16, 2012 at 4:16 PM, Hal Finkel <hfinkel at anl.gov>
> >> wrote:
> >> > Hello,
> >> >
> >> > As a followup, and partial solution, to PR14351, the attached
> >> > patch
> >> > removes some of the special significance of the first incoming
> >> > phi
> >> > value in the phi aliasing checking logic in BasicAA. In the
> >> > context of a loop, the current logic assumes that the first
> >> > incoming value is the interesting one (meaning that it is the
> >> > one
> >> > that comes from outside the loop), but this is often not the
> >> > case.
> >> > With this patch, we now test first the incoming value that comes
> >> > from a block other than the parent of the phi being tested. This
> >> > is important for, among other things, alias-analysis-based
> >> > instruction dependency breaking for instruction scheduling.
> >> >
> >> > As noted in the PR, Arnold is currently working on a more
> >> > general
> >> > solution. In the mean time, this fix captures several cases I
> >> > need
> >> > (and has an associated test case). Please review.
> >>
> >> --- a/lib/Analysis/BasicAliasAnalysis.cpp
> >> +++ b/lib/Analysis/BasicAliasAnalysis.cpp
> >> @@ -1065,9 +1065,15 @@ BasicAliasAnalysis::aliasPHI(const PHINode
> >> *PN,
> >> uint64_t PNSize,
> >>        if (PN > V2)
> >>          std::swap(Locs.first, Locs.second);
> >>
> >> +      // Find the first incoming phi value not from its parent.
> >> +      unsigned f = 0;
> >> +      for (; PN->getIncomingBlock(f) == PN->getParent(); ++f);
> >>
> >> Try to avoid for loops with an empty body, e.g. "while
> >> (PN->getIncomingBlock(f) == PN->getParent()) { ++f; }".  Or if you
> >> really like the for loop, at least put the ";" on a separate line.
> >>
> >> Also, this loop isn't guaranteed to terminate in unreachable code.
> >>
> >>
> >> It would be nice if you could simplify the testcase a bit more.
> >
> > Eli, thanks! I've turned to loop into a while (and fixed it so that
> > it won't run off the end of the list in unreachable code), and
> > cleanup up the test case. How's this?
> 
> LGTM.

r168245.

Thanks again,
Hal

> 
> -Eli
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory