[PATCH] D81004: [MemorySSA] Skip phi translation for phis in cycles (WIP).

[Florian Hahn via Phabricator via llvm-commits]

fhahn created this revision.
fhahn added reviewers: george.burgess.iv, asbirlea, efriedma.
Herald added subscribers: kosarev, Prazek.
Herald added a project: LLVM.

Currently we use the phi-translated address when traversing the
definitions upwards. The translated address is then used to check for
clobbers along a path. If the Phi is part of a cycle, this is not
correct I think, as it means we only check for clobbers with the initial
value of the cycle. But it is possible that a definition along the path
clobbers an address other than the initial value of the cycle.

This patch just detects trivial cycles, as I want to make sure I am not
missing anything before generalizing.

Consider the example below. Without this patch, MemorySSA will set the
defining access of `%lv = load i16, i16* %arrayidx, align 2, !tbaa !3`
to LiveOnEntry, because it phi-translates the location of the load to
`getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 1` in
%entry, but the loop body is executed twice and it loads from
`getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 1` andj
`getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 0`. The
latter aliases the store in %entry.

  define i32 @main(i32* noalias %ptr) {
  entry:
    %s1.ptr = getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 0
    store i16 1, i16* %s1.ptr, align 2, !tbaa !3
    br label %for.body
  
  for.body:
    %storemerge2 = phi i32 [ 1, %entry ], [ %dec, %for.body ]
    %idxprom1 = zext i32 %storemerge2 to i64
    %arrayidx = getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 %idxprom1
    %lv = load i16, i16* %arrayidx, align 2, !tbaa !3
    %conv = sext i16 %lv to i32
    store i32 %conv, i32* %ptr, align 4, !tbaa !7
    %dec = add nsw i32 %storemerge2, -1
    %cmp = icmp sgt i32 %storemerge2, 0
    br i1 %cmp, label %for.body, label %for.end
  
  for.end:
    %s2.ptr = getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 0
    store i16 0, i16* %s2.ptr, align 2, !tbaa !3
    ret i32 0
  }

Fixes PR46156.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81004

Files:
  llvm/include/llvm/Analysis/MemorySSA.h
  llvm/test/Analysis/MemorySSA/phi-translation-cycle.ll


Index: llvm/test/Analysis/MemorySSA/phi-translation-cycle.ll
===================================================================
--- /dev/null
+++ llvm/test/Analysis/MemorySSA/phi-translation-cycle.ll
@@ -0,0 +1,45 @@
+; RUN: opt -basicaa -tbaa -print-memoryssa -verify-memoryssa -analyze < %s 2>&1 | FileCheck %s --check-prefixes=CHECK
+; RUN: opt -aa-pipeline=basic-aa,type-based-aa -passes='print<memoryssa>,verify<memoryssa>' -disable-output < %s 2>&1 | FileCheck %s --check-prefixes=CHECK
+
+; Test case for PR46156. Make sure phi translation is skipped for cyclic phis.
+
+ at c = local_unnamed_addr global [2 x i16] zeroinitializer, align 2
+
+define i32 @test1(i32* noalias %ptr) {
+; CHECK-LABEL: define i32 @test1
+; CHECK-LABEL: entry:
+; CHECK:       ; 1 = MemoryDef(liveOnEntry)
+; CHECK-NEXT:  store i16 1, i16* %s1.ptr, align 2
+
+; CHECK-LABEL: %for.body
+; CHECK:       ; MemoryUse(1) MayAlias
+; CHECK-NEXT:    %lv = load i16, i16* %arrayidx, align 2, !tbaa !0
+
+entry:
+  %s1.ptr = getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 0
+  store i16 1, i16* %s1.ptr, align 2, !tbaa !0
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %storemerge2 = phi i32 [ 1, %entry ], [ %dec, %for.body ]
+  %idxprom1 = zext i32 %storemerge2 to i64
+  %arrayidx = getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 %idxprom1
+  %lv = load i16, i16* %arrayidx, align 2, !tbaa !0
+  %conv = sext i16 %lv to i32
+  store i32 %conv, i32* %ptr, align 4, !tbaa !4
+  %dec = add nsw i32 %storemerge2, -1
+  %cmp = icmp sgt i32 %storemerge2, 0
+  br i1 %cmp, label %for.body, label %for.end
+
+for.end:                                          ; preds = %for.body
+  %s2.ptr = getelementptr inbounds [2 x i16], [2 x i16]* @c, i64 0, i64 0
+  store i16 0, i16* %s2.ptr, align 2, !tbaa !0
+  ret i32 0
+}
+
+!0 = !{!1, !1, i64 0}
+!1 = !{!"short", !2, i64 0}
+!2 = !{!"omnipotent char", !3, i64 0}
+!3 = !{!"Simple C/C++ TBAA"}
+!4 = !{!5, !5, i64 0}
+!5 = !{!"int", !2, i64 0}
Index: llvm/include/llvm/Analysis/MemorySSA.h
===================================================================
--- llvm/include/llvm/Analysis/MemorySSA.h
+++ llvm/include/llvm/Analysis/MemorySSA.h
@@ -1217,7 +1217,18 @@
 private:
   void fillInCurrentPair() {
     CurrentPair.first = *DefIterator;
-    if (WalkingPhi && Location.Ptr) {
+    auto HasTrivialCycle = [](auto Def) {
+      BasicBlock *DefBlock = Def->getBlock();
+      auto *DefPhi = dyn_cast<MemoryPhi>(Def);
+      return DefPhi &&
+             any_of(DefPhi->blocks(), [DefBlock](BasicBlock *IncomingBlock) {
+               return DefBlock == IncomingBlock;
+             });
+    };
+    // Using a phi-translated address is only safe if the PHI is not part of a
+    // cycle. Otherwise we only check the address accessed in the first visit of
+    // the cycle.
+    if (WalkingPhi && Location.Ptr && !HasTrivialCycle(OriginalAccess)) {
       PHITransAddr Translator(
           const_cast<Value *>(Location.Ptr),
           OriginalAccess->getBlock()->getModule()->getDataLayout(), nullptr);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81004.267874.patch
Type: text/x-patch
Size: 3116 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200602/835fd44d/attachment.bin>