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

[Eli Friedman]

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.

-Eli