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

[Arnold Schwaighofer]

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
>> 
> 
> I don't entirely understand why the fix isn't to speculate less often in aliasPHI?

This is not about speculating. The error was in before we added speculating (it dates back to 2.7 or so). We are looking through phi nodes by analyzing its operands. We say noalias(V,  phi(v1, v2) if noalias(V, v1) and noalias(V, v2), which is useful in non-loop cfgs.

The problem is the code

// V is a Value *. V1Srcs contains the operands of a phi node.

1166	  // If all sources of the PHI node NoAlias or MustAlias V2, then returns
1167	  // NoAlias / MustAlias. Otherwise, returns MayAlias.
1168	  for (unsigned i = 1, e = V1Srcs.size(); i != e; ++i) {
1169	    Value *V = V1Srcs[i];
1170	
1171	    AliasResult ThisAlias = aliasCheck(V2, V2Size, V2TBAAInfo,
1172	                                       V, PNSize, PNTBAAInfo);
1173	    Alias = MergeAliasResults(ThisAlias, Alias);
1174	    if (Alias == MayAlias)
1175	      break;
1176	  }


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

> 
>> +bool BasicAliasAnalysis::isValueEqual(const Value *V, const Value *V2) {
>> +  if (V != V2)
>> +    return false;
>> +
>> +  const Instruction *Inst = dyn_cast<Instruction>(V);
>> +  if (!Inst)
>> +    return true;
>> +
>> +  // Use the dominance if available.
>> +  DT = getAnalysisIfAvailable<DominatorTree>();
>> +  if (DT) {
>> +    if (VisitedPhiBBs.size() > MaxNumPhiBBsValueDominanceCheck)
>> +      return false;
>> +
>> +    // Make sure that the visited phis are dominated by the Value.
>> +    for (SmallPtrSet<const BasicBlock *, 8>::iterator
>> +             PI = VisitedPhiBBs.begin(),
>> +             PE = VisitedPhiBBs.end();
>> +         PI != PE; ++PI)
>> +      if (!DT->dominates(Inst, *PI))
>> +        return false;
> 
> I haven't thought really hard about this, but did you want to use llvm::isPotentiallyReachable?

I’ll have a look at isPotentiallyReachable tomorrow. But yes, I think it can be formulated in terms of it. Something like:

>> +    for (SmallPtrSet<const BasicBlock *, 8>::iterator
>> +             PI = VisitedPhiBBs.begin(),
>> +             PE = VisitedPhiBBs.end();
>> +         PI != PE; ++PI)
>> +      if (isPotentiallyReachable(*PI->begin(), Inst))
>> +        return false;


> 
>> Modified: llvm/trunk/test/Analysis/BasicAA/2006-03-03-BadArraySubscript.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/BasicAA/2006-03-03-BadArraySubscript.ll?rev=198290&r1=198289&r2=198290&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Analysis/BasicAA/2006-03-03-BadArraySubscript.ll (original)
>> +++ llvm/trunk/test/Analysis/BasicAA/2006-03-03-BadArraySubscript.ll Wed Jan  1 21:31:36 2014
>> @@ -1,4 +1,4 @@
>> -; RUN: opt < %s -basicaa -aa-eval -disable-output 2>&1 | FileCheck %s
>> +; RUN: opt < %s -domtree -basicaa -aa-eval -disable-output 2>&1 | FileCheck %s
> 
> Adding -domtree to all these tests makes me very sad.

Using isPotentiallyReachable should make this unnecessary.

Thanks,
Arnold