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

[Arnold Schwaighofer]

Hi Nick,

thank you for the review.

On 8/30/2012 10:50 AM, Nick Lewycky wrote:
> Please remove the "IsAAPhiSpeculationEnabled" flag.

Ok

> +        // 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.
>

I copied this code (assert and comment) from further down in the same 
function. I will have another look at it.

Ah, I remember. The code checks that the base pointer returned by 
DecomposeGEPExpression and the value returned by GetUnderlyingObject 
agree. If they disagree it checks that TD == 0 otherwise it asserts 
stating that this should be the only reason they disagree.

How about I change the comments in the file to something like:
// DecomposeGEPExpression and GetUnderlyingObject should return the
// same result except when DecomposeGEPExpression has no TargetData.

> +        // Same offsets.
> +        if (GEP1BaseOffset-GEP2BaseOffset == 0 &&
> GEP1VariableIndices.empty() &&
> +            GEP2VariableIndices.empty())
>
> Why not "GEP1BaseOffset == GEP2BaseOffset"?


Ups. Yes sure.

> Also, isn't it fine if GEP1VariableIndices is equal to
> GEP2VariableIndices, where equal means "has the same elements in the
> same order"?

Yes.

> 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.

I will have a look.

> +      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'.

Will address those.

> 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.

Okay I will update the comment.

> 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.

I will run clang bootstrap. (This patch has gone through an extensive 
set of tests we use for testing arm and hexagon - that is not to say 
more tests could not find a problem).

> 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
>

This is odd, I am using thunderbird and followed the instructions on 
http://llvm.org/docs/DeveloperPolicy.html#making-a-patch. If I go to 
Options> Advanced>General>Config Editor mail.content_disposition_type is 
set to 1. I guess I have to debug my configuration/usage of Thunderbird. 
Sorry about that.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation