[llvm] r198290 - BasicAA: Fix value equality and phi cycles

[Nick Lewycky]

Arnold Schwaighofer wrote:
>
> On Jan 1, 2014, at 8:24 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>
>> On 01/01/2014 07:31 PM, Arnold Schwaighofer wrote:
>>> Author: arnolds
>>> Date: Wed Jan  1 21:31:36 2014
>>> New Revision: 198290

>>>     // Are we checking for alias of the same value?
>>> -  if (V1 == V2) return MustAlias;
>>> +  if (isValueEqual(V1, V2)) return MustAlias;
>>
>> This demands a massive explanation of what on earth is going on
>
>> Of course %x mustalias %x! (Yes, I've read the PR and understand it what's going on, but I'd much rather see code that explicitly dealt with the "we're in a loop and speculating on PHIs case" broken out.)
>
> At the top of the file I put the comment:
>
> 42	/// Cutoff after which to stop analysing a set of phi nodes potentially involved
> 43	/// in a cycle. Because we are analysing 'through' phi nodes we need to be
> 44	/// careful with value equivalence. We use dominance to make sure a value cannot
> 45	/// be involved in a cycle.
> 46	const unsigned MaxNumPhiBBsValueDominanceCheck = 20;
>
> The function isValueEqual has the comment:
>
> +    /// \brief Check whether two Values can be considered equivalent.
> +    ///
> +    /// In addition to pointer equivalence of \p V1 and \p V2 this checks
> +    /// whether they can not be part of a cycle in the value graph by looking at
> +    /// all visited phi nodes an making sure that the value dominates all of
> +    /// them.
> +    bool isValueEqual(const Value *V1, const Value *V2);
>
> The variable VisitedPhiBBs has the comment:
>
>
> 508	    /// \brief Track phi nodes we have visited. When interpret "Value" pointer
> 509	    /// equality as value equality we need to make sure that the "Value" is not
> 510	    /// part of a cycle. Otherwise, two uses could come from different
> 511	    /// "iterations" of a cycle and see different values for the same "Value"
> 512	    /// pointer.
> 513	    /// The following example shows the problem:
> 514	    ///   %p = phi(%alloca1, %addr2)
> 515	    ///   %l = load %ptr
> 516	    ///   %addr1 = gep, %alloca2, 0, %l
> 517	    ///   %addr2 = gep  %alloca2, 0, (%l + 1)
> 518	    ///      alias(%p, %addr1) ->  MayAlias !
> 519	    ///   store %l, ...
> 520	    SmallPtrSet<const BasicBlock*, 8>  VisitedPhiBBs;
>
>
> I can add another comment at isValueEqual's use?

Yes, that's where I wanted the comment.

Given the API that alias analysis has, alias(%x, %x) == mustalias. There 
are ways that could be false (analyzing %x across two different call 
sites to the same function), but for the subset of alias queries we can 
express in the AliasAnalysis API, it has to be true.

Now here we are in a function named aliasCheck(V1, V2) which has always 
been attempting to answer the question "does V1 alias V2?". If V1 == V2 
does not lead to a mustalias answer, then this function *isn't* actually 
answering alias queries any more, it's doing something else, different, 
surprising. That's my problem.

This whole VisitedPhiBBs thing doesn't belong in aliasCheck. aliasPHI 
needs to check some property of the two pointers that isn't the aliasing 
property -- at least, not the aliasing property as defined by LLVM's 
AliasAnalysis API. It could be called aliasing in the literature, but 
it's some sort of CFG-aware aliasing test. I don't really know what it is.

Nick