[llvm-commits] [PATCH] Extend BasicAliasAnalysis to recognize cyclic NoAlias phis in loops

[Nick Lewycky]

Arnold Schwaighofer wrote:
> I have split the patch into two(GEP of noalias and cyclic noalias
> phis), addressed your comments, bootstrapped clang and ran make
> check-all and the test-suite.
>
> This does not fix PR11331.

Drat :)

> Okay to commit?

0001 looks perfect. Please commit!

For 0002:

+        // Pretend the phis do not alias.
+        assert(AliasCache.count(Locs));

Please add some text here, the form ''assert(... && "string")'' is common.

With that, please commit!

Nick

> On Thu, Aug 30, 2012 at 10:50 AM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>> On 08/30/2012 07:48 AM, Arnold Schwaighofer wrote:
>>>
>>> Ping.
>>
>>
>> Please remove the "IsAAPhiSpeculationEnabled" flag.
>>
>> +        // If DecomposeGEPExpression isn't able to look all the way through
>> the
>> +        // addressing operation, we must not have TD and this is too
>> complex for
>> +        // us to handle without it.
>>
>> Huh? There's plenty other reasons DecomposeGEPExpression might fail. It
>> might encounter a weak alias for instance, starting at the top of the
>> function.
>>
>> +        // Same offsets.
>> +        if (GEP1BaseOffset-GEP2BaseOffset == 0&&
>> GEP1VariableIndices.empty()&&
>> +            GEP2VariableIndices.empty())
>>
>> Why not "GEP1BaseOffset == GEP2BaseOffset"?
>>
>> Also, isn't it fine if GEP1VariableIndices is equal to GEP2VariableIndices,
>> where equal means "has the same elements in the same order"?
>>
>> Could you write a unit test for just this enhancement to GEP analysis, and
>> land it separately? The PHI transform seems more difficult to reason about.
>>
>> +      LocPair Locs(Location(PN, PNSize, PNTBAAInfo),
>> +                     Location(V2, V2Size, V2TBAAInfo));
>>
>> The second line should line up with the 'Location' on the first.
>>
>> +      if  (IsAAPhiSpeculationEnabled&&  Alias == NoAlias) {
>>
>> Two is too many spaces after 'if'.
>>
>> Okay, I finally convinced myself that your patch is right. I think you
>> should rework the comment block:
>> +      // Check for phis in loops where we know that the incoming value from
>> +      // outside the loop is NoAlias and the operations inside the loop
>> that are
>> +      // feeding the phis are congruent.
>> +      // bb:
>> +      //    ptr = ptr2 + 1
>> +      // loop:
>> +      //    ptr_phi = phi [bb, ptr], [loop, ptr_plus_one]
>> +      //    ptr2_phi = phi [bb, ptr2], [loop, ptr2_plus_one]
>> +      //    ...
>> +      //    ptr_plus_one = gep ptr_phi, 1
>> +      //    ptr2_plus_one = gep ptr2_phi, 1
>> +      // We assume that the the phis (ptr_phi, ptr2_phi) do not alias each
>> other
>> +      // and try to prove otherwise.
>> because it makes wonder where we've proven that the phi is a loop.
>>
>> The comment that's already there:
>>    // If all sources of the PHI node NoAlias or MustAlias V2, then returns
>>    // NoAlias / MustAlias. Otherwise, returns MayAlias.
>> gives the kernel of the idea; the result of the alias query against the PHI
>> is the sum of the phi's inputs. What you're adding is that if the PHI's
>> input is itself (through some amount of recursion) then that doesn't add any
>> new information, so you just mark it NoAlias.
>>
>> Make sure you run a larger test (like a clang bootstrap) before committing
>> the PHI part, but I don't think it needs further review before committing.
>> Also please check whether this fixes PR11331 and close that if so.
>>
>> Nick
>>
>> PS. There's something wrong with the way you're attaching the file --
>> possibly using "Content-disposition: inline" MIME header? See how it isn't
>> an attachment here:
>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120827/149338.html
>> . There's some text about this problem here:
>> http://llvm.org/docs/DeveloperPolicy.html#making-a-patch
>>
>>
>>>
>>> On 8/9/2012 10:54 AM, Arnold Schwaighofer wrote:
>>>>
>>>> Hi,
>>>>
>>>> BasicAliasAnalysis should recognize phis that have incoming values from
>>>> outside the loop that are known to not alias and whose other incoming
>>>> values do not change this fact because they perform congruent operations.
>>>>
>>>> Example: We want to prove that ptr_phi and ptr_phi2 do not alias each
>>>> other.
>>>>
>>>> bb:
>>>> ptr = ptr2 + 1
>>>>
>>>> loop:
>>>> ptr_phi = phi [bb, ptr], [loop, ptr_plus_one]
>>>> ptr2_phi = phi [bb, ptr2], [loop, ptr2_plus_one]
>>>> ...
>>>> ptr_plus_one = gep ptr_phi, 1
>>>> ptr2_plus_one = gep ptr2_phi, 1
>>>>
>>>> This would enable the elimination of one load in code like the following:
>>>>
>>>> extern int foo;
>>>>
>>>> int test_noalias(int *ptr, int num, int* coeff) {
>>>> int *ptr2 = ptr;
>>>> int result = (*ptr++) * (*coeff--);
>>>> while (num--) {
>>>> *ptr2++ = *ptr;
>>>> result += (*coeff--) * (*ptr++);
>>>> }
>>>> *ptr = foo;
>>>> return result;
>>>> }
>>>>
>>>> Currently, without the improvement to basic alias analysis we generate
>>>> the following code for the loop:
>>>>
>>>> The loads from %r9 are the accesses to "ptr".
>>>>
>>>> .LBB0_2: # %while.body
>>>> # =>This Inner Loop Header:
>>>> Depth=1
>>>> movl (%r9), %ecx
>>>> movl %ecx, -4(%r9)
>>>> movl (%r9), %ecx
>>>> imull (%rdx), %ecx
>>>> addl %ecx, %eax
>>>> addq $-4, %rdx
>>>> addq $4, %r9
>>>> decl %esi
>>>> jne .LBB0_2
>>>>
>>>> With the improvement we would generate:
>>>>
>>>> .LBB0_2: # %while.body
>>>> # =>This Inner Loop Header:
>>>> Depth=1
>>>> movl (%r9), %ecx
>>>> movl %ecx, -4(%r9)
>>>> imull (%rdx), %ecx
>>>> addl %ecx, %eax
>>>> addq $4, %r9
>>>> addq $-4, %rdx
>>>> decl %esi
>>>> jne .LBB0_2
>>>>
>>>> The attached patch implements this enhancement.
>>>>
>>>> http://llvm.org/bugs/show_bug.cgi?id=13564
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits