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

Wed Sep 5 13:49:29 PDT 2012

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.

Okay to commit?

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-BasicAA-GEPs-of-NoAlias-ing-base-ptr-with-equivalent.patch
Type: application/octet-stream
Size: 9127 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120905/a70207d2/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-BasicAA-Recognize-cyclic-NoAlias-phis.patch
Type: application/octet-stream
Size: 5089 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120905/a70207d2/attachment-0001.obj>