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

[Nick Lewycky]

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