[PATCH] D115113: [InstCombine] Do not combine atomic and non-atomic loads.

[Ricky Zhou via Phabricator via llvm-commits]

rickyz updated this revision to Diff 395600.
rickyz marked an inline comment as done.
rickyz added a comment.

Respond to comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115113/new/

https://reviews.llvm.org/D115113

Files:
  llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
  llvm/test/Transforms/InstCombine/phi.ll


Index: llvm/test/Transforms/InstCombine/phi.ll
===================================================================
--- llvm/test/Transforms/InstCombine/phi.ll
+++ llvm/test/Transforms/InstCombine/phi.ll
@@ -547,6 +547,29 @@
   ret i32 %res
 }
 
+; Atomic and non-atomic loads should not be combined.
+define i32 @PR51435(i32* %ptr, i32* %atomic_ptr, i1 %c) {
+; CHECK-LABEL: @PR51435(
+; CHECK:       entry:
+; CHECK-NEXT:    [[NON_ATOMIC:%.*]] = load i32, i32* %ptr, align 4
+; CHECK:       if:
+; CHECK-NEXT:    [[ATOMIC:%.*]] = load atomic i32, i32* %atomic_ptr acquire, align 4
+; CHECK:       end:
+; CHECK-NEXT:    [[COND:%.*]] = phi i32 [ [[NON_ATOMIC]], %entry ], [ [[ATOMIC]], %if ]
+; CHECK-NEXT:    ret i32 [[COND]]
+entry:
+  %x = load i32, i32* %ptr, align 4
+  br i1 %c, label %if, label %end
+
+if:
+  %y = load atomic i32, i32* %atomic_ptr acquire, align 4
+  br label %end
+
+end:
+  %cond = phi i32 [ %x, %entry ], [ %y, %if ]
+  ret i32 %cond
+}
+
 define i1 @test18(i1 %cond) {
 ; CHECK-LABEL: @test18(
 ; CHECK-NEXT:    br i1 [[COND:%.*]], label [[TRUE:%.*]], label [[FALSE:%.*]]
Index: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
@@ -662,7 +662,8 @@
 
   // FIXME: This is overconservative; this transform is allowed in some cases
   // for atomic operations.
-  if (FirstLI->isAtomic())
+  const bool IsAtomic = FirstLI->isAtomic();
+  if (IsAtomic)
     return nullptr;
 
   // When processing loads, we need to propagate two bits of information to the
@@ -670,9 +671,9 @@
   // don't sink loads when some have their alignment specified and some don't.
   // visitLoadInst will propagate an alignment onto the load when TD is around,
   // and if TD isn't around, we can't handle the mixed case.
-  bool IsVolatile = FirstLI->isVolatile();
+  const bool IsVolatile = FirstLI->isVolatile();
   Align LoadAlignment = FirstLI->getAlign();
-  unsigned LoadAddrSpace = FirstLI->getPointerAddressSpace();
+  const unsigned LoadAddrSpace = FirstLI->getPointerAddressSpace();
 
   // We can't sink the load if the loaded value could be modified between the
   // load and the PHI.
@@ -687,7 +688,6 @@
       FirstLI->getParent()->getTerminator()->getNumSuccessors() != 1)
     return nullptr;
 
-  // Check to see if all arguments are the same operation.
   for (auto Incoming : drop_begin(zip(PN.blocks(), PN.incoming_values()))) {
     BasicBlock *InBB = std::get<0>(Incoming);
     Value *InVal = std::get<1>(Incoming);
@@ -695,11 +695,14 @@
     if (!LI || !LI->hasOneUser())
       return nullptr;
 
+    // Make sure all arguments are the same type of operation.
+    if (LI->isAtomic() != IsAtomic || LI->isVolatile() != IsVolatile ||
+        LI->getPointerAddressSpace() != LoadAddrSpace)
+      return nullptr;
+
     // We can't sink the load if the loaded value could be modified between
     // the load and the PHI.
-    if (LI->isVolatile() != IsVolatile || LI->getParent() != InBB ||
-        LI->getPointerAddressSpace() != LoadAddrSpace ||
-        !isSafeAndProfitableToSinkLoad(LI))
+    if (LI->getParent() != InBB || !isSafeAndProfitableToSinkLoad(LI))
       return nullptr;
 
     LoadAlignment = std::min(LoadAlignment, Align(LI->getAlign()));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D115113.395600.patch
Type: text/x-patch
Size: 3370 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211221/230ef5ea/attachment.bin>