[JumpThreading] PRE unordered loads

[sanjoy]

Summary

Extend JumpThreading's PRE to unordered atomic loads.

[majnemer]

lib/Transforms/Scalar/JumpThreading.cpp

927 ↗

(On Diff #63888)

This comment seems wrong now.

[sanjoy]

• address review comments

[jfb]

Could you test that:

• non-atomic loads aren't forwarded to atomic loads
• non-atomic stores aren't forwarded to atomic loads (this seems useless, maybe it's valid?)

lgtm otherwise.

lib/Transforms/Scalar/JumpThreading.cpp

1059 ↗

(On Diff #63892)

Li->isVolatile() seems simpler than having the comment.

[sanjoy]

Will add the negative tests as you suggest.

lib/Transforms/Scalar/JumpThreading.cpp

1059 ↗

(On Diff #63892)

That sounds like an unrelated (to this patch) change. I've removed the /*isVolatile*/ comment for now (so that bit stays the same), let me know if you'd like me to s/false/LI->isVolatile()/ separately.

llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp

bool JumpThreadingPass::ProcessImpliedCondition(BasicBlock *BB) {
   return false;
 }
 
 /// SimplifyPartiallyRedundantLoad - If LI is an obviously partially redundant
 /// load instruction, eliminate it by replacing it with a PHI node.  This is an
 /// important optimization that encourages jump threading, and needs to be run
 /// interlaced with other jump threading tasks.
 bool JumpThreadingPass::SimplifyPartiallyRedundantLoad(LoadInst *LI) {
-  // Don't hack volatile/atomic loads.
-  if (!LI->isSimple()) return false;
+  // Don't hack volatile and ordered loads.
+  if (!LI->isUnordered()) return false;
 
   // If the load is defined in a block with exactly one predecessor, it can't be
   // partially redundant.
   BasicBlock *LoadBB = LI->getParent();
   if (LoadBB->getSinglePredecessor())
     return false;
 
   // If the load is defined in an EH pad, it can't be partially redundant,
bool JumpThreadingPass::SimplifyPartiallyRedundantLoad(LoadInst *LI) {
   }
 
   // If the value isn't available in all predecessors, then there will be
   // exactly one where it isn't available.  Insert a load on that edge and add
   // it to the AvailablePreds list.
   if (UnavailablePred) {
     assert(UnavailablePred->getTerminator()->getNumSuccessors() == 1 &&
            "Can't handle critical edge here!");
-    LoadInst *NewVal = new LoadInst(LoadedPtr, LI->getName()+".pr", false,
-                                 LI->getAlignment(),
-                                 UnavailablePred->getTerminator());
+    LoadInst *NewVal =
+        new LoadInst(LoadedPtr, LI->getName() + ".pr", false,
+                     LI->getAlignment(), LI->getOrdering(), LI->getSynchScope(),
     NewVal->setDebugLoc(LI->getDebugLoc());
     if (AATags)
       NewVal->setAAMetadata(AATags);
 
     AvailablePreds.push_back(std::make_pair(UnavailablePred, NewVal));
   }
 
   // Now we know that each predecessor of this block has a value in

llvm/trunk/test/Transforms/JumpThreading/thread-loads.ll

 
 if.then60:
   ret i32 42
 
 return:
   ret i32 13
 }
 
+define i32 @test4(i32* %P) {
+; CHECK-LABEL: @test4(
+entry:
+  %v0 = tail call i32 (...) @f1()
+  %v1 = icmp eq i32 %v0, 0
+  br i1 %v1, label %bb1, label %bb
+
+bb:
+; CHECK: bb1.thread:
+; CHECK: store atomic
+; CHECK: br label %bb3
+  store atomic i32 42, i32* %P unordered, align 4
+  br label %bb1
+
+bb1:
+; CHECK: bb1:
+; CHECK-NOT: phi
+; CHECK: load atomic
+  %res.0 = phi i32 [ 1, %bb ], [ 0, %entry ]
+  %v2 = load atomic i32, i32* %P unordered, align 4
+  %v3 = icmp sgt i32 %v2, 36
+  br i1 %v3, label %bb3, label %bb2
+
+bb2:
+  %v4 = tail call i32 (...) @f2()
+  ret i32 %res.0
+
+bb3:
+  ret i32 %res.0
+}
+
+define i32 @test5(i32* %P) {
+; Negative test
+
+; CHECK-LABEL: @test5(
+entry:
+  %v0 = tail call i32 (...) @f1()
+  %v1 = icmp eq i32 %v0, 0
+  br i1 %v1, label %bb1, label %bb
+
+bb:
+; CHECK: bb:
+; CHECK-NEXT:   store atomic i32 42, i32* %P release, align 4
+; CHECK-NEXT:   br label %bb1
+  store atomic i32 42, i32* %P release, align 4
+  br label %bb1
+
+bb1:
+; CHECK: bb1:
+; CHECK-NEXT:  %res.0 = phi i32 [ 1, %bb ], [ 0, %entry ]
+; CHECK-NEXT:  %v2 = load atomic i32, i32* %P acquire, align 4
+; CHECK-NEXT:  %v3 = icmp sgt i32 %v2, 36
+; CHECK-NEXT:  br i1 %v3, label %bb3, label %bb2
+
+  %res.0 = phi i32 [ 1, %bb ], [ 0, %entry ]
+  %v2 = load atomic i32, i32* %P acquire, align 4
+  %v3 = icmp sgt i32 %v2, 36
+  br i1 %v3, label %bb3, label %bb2
+
+bb2:
+  %v4 = tail call i32 (...) @f2()
+  ret i32 %res.0
+
+bb3:
+  ret i32 %res.0
+}
+
+define i32 @test6(i32* %P) {
+; Negative test
+
+; CHECK-LABEL: @test6(
+entry:
+  %v0 = tail call i32 (...) @f1()
+  %v1 = icmp eq i32 %v0, 0
+  br i1 %v1, label %bb1, label %bb
+
+bb:
+; CHECK: bb:
+; CHECK-NEXT:   store i32 42, i32* %P
+; CHECK-NEXT:   br label %bb1
+  store i32 42, i32* %P
+  br label %bb1
+
+bb1:
+; CHECK: bb1:
+; CHECK-NEXT:  %res.0 = phi i32 [ 1, %bb ], [ 0, %entry ]
+; CHECK-NEXT:  %v2 = load atomic i32, i32* %P acquire, align 4
+; CHECK-NEXT:  %v3 = icmp sgt i32 %v2, 36
+; CHECK-NEXT:  br i1 %v3, label %bb3, label %bb2
+
+  %res.0 = phi i32 [ 1, %bb ], [ 0, %entry ]
+  %v2 = load atomic i32, i32* %P acquire, align 4
+  %v3 = icmp sgt i32 %v2, 36
+  br i1 %v3, label %bb3, label %bb2
+
+bb2:
+  %v4 = tail call i32 (...) @f2()
+  ret i32 %res.0
+
+bb3:
+  ret i32 %res.0
+}
+
+define i32 @test7(i32* %P) {
+; Negative test
+
+; CHECK-LABEL: @test7(
+entry:
+  %v0 = tail call i32 (...) @f1()
+  %v1 = icmp eq i32 %v0, 0
+  br i1 %v1, label %bb1, label %bb
+
+bb:
+; CHECK: bb:
+; CHECK-NEXT:   %val = load i32, i32* %P
+; CHECK-NEXT:   br label %bb1
+  %val = load i32, i32* %P
+  br label %bb1
+
+bb1:
+; CHECK: bb1:
+; CHECK-NEXT:  %res.0 = phi i32 [ 1, %bb ], [ 0, %entry ]
+; CHECK-NEXT:  %v2 = load atomic i32, i32* %P acquire, align 4
+; CHECK-NEXT:  %v3 = icmp sgt i32 %v2, 36
+; CHECK-NEXT:  br i1 %v3, label %bb3, label %bb2
+
+  %res.0 = phi i32 [ 1, %bb ], [ 0, %entry ]
+  %v2 = load atomic i32, i32* %P acquire, align 4
+  %v3 = icmp sgt i32 %v2, 36
+  br i1 %v3, label %bb3, label %bb2
+
+bb2:
+  %v4 = tail call i32 (...) @f2()
+  ret i32 %res.0
+
+bb3:
+  ret i32 %res.0
+}
+
 !0 = !{!3, !3, i64 0}
 !1 = !{!"omnipotent char", !2}
 !2 = !{!"Simple C/C++ TBAA", null}
 !3 = !{!"int", !1}