[MemorySSA] Reset location size if IsGuaranteedLoopInvariant after phi tranlation

[StephenFan]

Summary

We set the Location size to beforeOrAfter if the Location value is not
guaranteed loop invariant. But in some cases, we need to reset the
location size if the location size is precise after phi tranlation of
location value. This will improve MemorySSA analysis results.

[StephenFan]

Rebase.

[nikic]

llvm/include/llvm/Analysis/MemorySSA.h

1255

The logic here looks somewhat convoluted -- isn't this equivalent to doing the translation first and then doing the invariance check afterwards (dropping the size adjustment before phi translation entirely)?

[StephenFan]

llvm/include/llvm/Analysis/MemorySSA.h

1255

Only if the phi translator gets a value different from the original one, line 1271 would be executed. if drop the size adjustment before phi translation entirely, in the case that Location.Ptr is not guaranteed loop invariant and can not be phi translated, the location size of Location.Ptr would not be adjusted to beforeOrAfterPointer. I hope I understood your question correctly and answered it clearly. :)

[nikic]

llvm/include/llvm/Analysis/MemorySSA.h

1255

Maybe it's easier to explain with code, this is what I had in mind:

void fillInCurrentPair() {
  CurrentPair.first = *DefIterator;
  CurrentPair.second = Location;
  if (WalkingPhi && Location.Ptr) {
    PHITransAddr Translator(
        const_cast<Value *>(Location.Ptr),
        OriginalAccess->getBlock()->getModule()->getDataLayout(), nullptr);

    if (!Translator.PHITranslateValue(OriginalAccess->getBlock(),
                                      DefIterator.getPhiArgBlock(), DT, true))
      CurrentPair.second =
          CurrentPair.second.getWithNewPtr(Translator.getAddr());

    // Mark size as unknown, if the location is not guaranteed to be
    // loop-invariant for any possible loop in the function. Setting the size
    // to unknown guarantees that any memory accesses that access locations
    // after the pointer are considered as clobbers, which is important to
    // catch loop carried dependences.
    if (!IsGuaranteedLoopInvariant(CurrentPair.second.Ptr))
      CurrentPair.second =
          Location.getWithNewSize(LocationSize::beforeOrAfterPointer());
  }
}

[StephenFan]

Thanks nikic for digging into to reduce the logic.

[nikic]

LGTM

llvm/include/llvm/Analysis/MemorySSA.h

private:
   /// loop. In particular, this guarantees that it only references a single
   /// MemoryLocation during execution of the containing function.
   bool IsGuaranteedLoopInvariant(const Value *Ptr) const;
 
   void fillInCurrentPair() {
     CurrentPair.first = *DefIterator;
     CurrentPair.second = Location;
     if (WalkingPhi && Location.Ptr) {
-      // Mark size as unknown, if the location is not guaranteed to be
-      // loop-invariant for any possible loop in the function. Setting the size
-      // to unknown guarantees that any memory accesses that access locations
-      // after the pointer are considered as clobbers, which is important to
-      // catch loop carried dependences.
-      if (!IsGuaranteedLoopInvariant(Location.Ptr))
-        CurrentPair.second =
-            Location.getWithNewSize(LocationSize::beforeOrAfterPointer());
       PHITransAddr Translator(
           const_cast<Value *>(Location.Ptr),
           OriginalAccess->getBlock()->getModule()->getDataLayout(), nullptr);
 
       if (!Translator.PHITranslateValue(OriginalAccess->getBlock(),
-                                        DefIterator.getPhiArgBlock(), DT,
-                                        true)) {
-        Value *TransAddr = Translator.getAddr();
-        if (TransAddr != Location.Ptr) {
-          CurrentPair.second = CurrentPair.second.getWithNewPtr(TransAddr);
+                                        DefIterator.getPhiArgBlock(), DT, true))
+        if (Translator.getAddr() != CurrentPair.second.Ptr)
+          CurrentPair.second =
+              CurrentPair.second.getWithNewPtr(Translator.getAddr());
 
-          if (TransAddr &&
-              !IsGuaranteedLoopInvariant(TransAddr))
-            CurrentPair.second = CurrentPair.second.getWithNewSize(
-                LocationSize::beforeOrAfterPointer());
-        }
-      }
-    }
-  }
+      // Mark size as unknown, if the location is not guaranteed to be
+      // loop-invariant for any possible loop in the function. Setting the size
+      // to unknown guarantees that any memory accesses that access locations
+      // after the pointer are considered as clobbers, which is important to
+      // catch loop carried dependences.
+      if (!IsGuaranteedLoopInvariant(CurrentPair.second.Ptr))
 
   MemoryAccessPair CurrentPair;
   memoryaccess_def_iterator DefIterator;
   MemoryLocation Location;
   MemoryAccess *OriginalAccess = nullptr;
   DominatorTree *DT = nullptr;
   bool WalkingPhi = false;
 };

llvm/test/Analysis/MemorySSA/phi-translation.ll

 
 ; CHECK-LABEL: rhs:
 ; CHECK:       ; 2 = MemoryDef(liveOnEntry)
 ; CHECK-NEXT:  store i16 1, i16* %s2.ptr, align 2
 
 ; CHECK-LABEL: merge:
 ; CHECK:       ; 3 = MemoryPhi({lhs,1},{rhs,2})
 ; CHECK-NEXT   %storemerge2 = phi i32 [ 2, %lhs ], [ 3, %rhs ]
-; TODO: Here the defining access could be liveOnEntry. Since all incoming
-; values of phi node can be phi translated.
-; CHECK:       ; MemoryUse(3)
-; CHECK-NEXT:    %lv = load i16, i16* %arrayidx, align 2
+; LIMIT:       ; MemoryUse(3)
+; LIMIT-NEXT:  %lv = load i16, i16* %arrayidx, align 2
+; NOLIMIT:     ; MemoryUse(liveOnEntry)
+; NOLIMIT-NEXT: %lv = load i16, i16* %arrayidx, align 2
 
 entry:
   br i1 %cmp, label %lhs, label %rhs
 
 lhs:
   %s1.ptr = getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 0
   store i16 1, i16* %s1.ptr, align 2
   br label %merge