Revert "[MergeICmps] Don't require GEP"

This reverts commit e7fb1c1. Causes crashes, see https://reviews.llvm.org/rGe7fb1c15cb85d748c1c4fdd5a2eb5613ec7bef1d.
llvm · Mar 3, 2022 · bc1574b · bc1574b
1 parent 19c1b08
commit bc1574b
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 37 deletions.
diff --git a/llvm/lib/Transforms/Scalar/MergeICmps.cpp b/llvm/lib/Transforms/Scalar/MergeICmps.cpp
@@ -144,33 +144,31 @@ BCEAtom visitICmpLoadOperand(Value *const Val, BaseIdentifier &BaseId) {
     LLVM_DEBUG(dbgs() << "volatile or atomic\n");
     return {};
   }
-  Value *Addr = LoadI->getOperand(0);
+  Value *const Addr = LoadI->getOperand(0);
   if (Addr->getType()->getPointerAddressSpace() != 0) {
     LLVM_DEBUG(dbgs() << "from non-zero AddressSpace\n");
     return {};
   }
-  const auto &DL = LoadI->getModule()->getDataLayout();
-  if (!isDereferenceablePointer(Addr, LoadI->getType(), DL)) {
+  auto *const GEP = dyn_cast<GetElementPtrInst>(Addr);
+  if (!GEP)
+    return {};
+  LLVM_DEBUG(dbgs() << "GEP\n");
+  if (GEP->isUsedOutsideOfBlock(LoadI->getParent())) {
+    LLVM_DEBUG(dbgs() << "used outside of block\n");
+    return {};
+  }
+  const auto &DL = GEP->getModule()->getDataLayout();
+  if (!isDereferenceablePointer(GEP, LoadI->getType(), DL)) {
     LLVM_DEBUG(dbgs() << "not dereferenceable\n");
     // We need to make sure that we can do comparison in any order, so we
     // require memory to be unconditionnally dereferencable.
     return {};
   }
-
-  APInt Offset = APInt(DL.getPointerTypeSizeInBits(Addr->getType()), 0);
-  Value *Base = Addr;
-  auto *GEP = dyn_cast<GetElementPtrInst>(Addr);
-  if (GEP) {
-    LLVM_DEBUG(dbgs() << "GEP\n");
-    if (GEP->isUsedOutsideOfBlock(LoadI->getParent())) {
-      LLVM_DEBUG(dbgs() << "used outside of block\n");
-      return {};
-    }
-    if (!GEP->accumulateConstantOffset(DL, Offset))
-      return {};
-    Base = GEP->getPointerOperand();
-  }
-  return BCEAtom(GEP, LoadI, BaseId.getBaseId(Base), Offset);
+  APInt Offset = APInt(DL.getPointerTypeSizeInBits(GEP->getType()), 0);
+  if (!GEP->accumulateConstantOffset(DL, Offset))
+    return {};
+  return BCEAtom(GEP, LoadI, BaseId.getBaseId(GEP->getPointerOperand()),
+                 Offset);
 }
 
 // A comparison between two BCE atoms, e.g. `a == o.a` in the example at the
@@ -370,11 +368,8 @@ Optional<BCECmpBlock> visitCmpBlock(Value *const Val, BasicBlock *const Block,
     return None;
 
   BCECmpBlock::InstructionSet BlockInsts(
-      {Result->Lhs.LoadI, Result->Rhs.LoadI, Result->CmpI, BranchI});
-  if (Result->Lhs.GEP)
-    BlockInsts.insert(Result->Lhs.GEP);
-  if (Result->Rhs.GEP)
-    BlockInsts.insert(Result->Rhs.GEP);
+      {Result->Lhs.GEP, Result->Rhs.GEP, Result->Lhs.LoadI, Result->Rhs.LoadI,
+       Result->CmpI, BranchI});
   return BCECmpBlock(std::move(*Result), Block, BlockInsts);
 }
 
@@ -609,15 +604,8 @@ static BasicBlock *mergeComparisons(ArrayRef<BCECmpBlock> Comparisons,
                          NextCmpBlock->getParent(), InsertBefore);
   IRBuilder<> Builder(BB);
   // Add the GEPs from the first BCECmpBlock.
-  Value *Lhs, *Rhs;
-  if (FirstCmp.Lhs().GEP)
-    Lhs = Builder.Insert(FirstCmp.Lhs().GEP->clone());
-  else
-    Lhs = FirstCmp.Lhs().LoadI->getPointerOperand();
-  if (FirstCmp.Rhs().GEP)
-    Rhs = Builder.Insert(FirstCmp.Rhs().GEP->clone());
-  else
-    Rhs = FirstCmp.Rhs().LoadI->getPointerOperand();
+  Value *const Lhs = Builder.Insert(FirstCmp.Lhs().GEP->clone());
+  Value *const Rhs = Builder.Insert(FirstCmp.Rhs().GEP->clone());
 
   Value *IsEqual = nullptr;
   LLVM_DEBUG(dbgs() << "Merging " << Comparisons.size() << " comparisons -> "

diff --git a/llvm/test/Transforms/MergeICmps/X86/opaque-ptr.ll b/llvm/test/Transforms/MergeICmps/X86/opaque-ptr.ll
@@ -7,12 +7,21 @@ target triple = "x86_64-unknown-unknown"
 
 define i1 @test(ptr dereferenceable(8) %a, ptr dereferenceable(8) %b) {
 ; CHECK-LABEL: @test(
-; CHECK-NEXT:  "entry+land.rhs.i":
-; CHECK-NEXT:    [[MEMCMP:%.*]] = call i32 @memcmp(ptr [[A:%.*]], ptr [[B:%.*]], i64 8)
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp eq i32 [[MEMCMP]], 0
-; CHECK-NEXT:    br label [[OPEQ1_EXIT:%.*]]
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[V0:%.*]] = load i32, ptr [[A:%.*]], align 4
+; CHECK-NEXT:    [[V1:%.*]] = load i32, ptr [[B:%.*]], align 4
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq i32 [[V0]], [[V1]]
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[LAND_RHS_I:%.*]], label [[OPEQ1_EXIT:%.*]]
+; CHECK:       land.rhs.i:
+; CHECK-NEXT:    [[SECOND_I:%.*]] = getelementptr inbounds [[S:%.*]], ptr [[A]], i64 0, i32 1
+; CHECK-NEXT:    [[V2:%.*]] = load i32, ptr [[SECOND_I]], align 4
+; CHECK-NEXT:    [[SECOND2_I:%.*]] = getelementptr inbounds [[S]], ptr [[B]], i64 0, i32 1
+; CHECK-NEXT:    [[V3:%.*]] = load i32, ptr [[SECOND2_I]], align 4
+; CHECK-NEXT:    [[CMP3_I:%.*]] = icmp eq i32 [[V2]], [[V3]]
+; CHECK-NEXT:    br label [[OPEQ1_EXIT]]
 ; CHECK:       opeq1.exit:
-; CHECK-NEXT:    ret i1 [[TMP0]]
+; CHECK-NEXT:    [[PHI:%.*]] = phi i1 [ false, [[ENTRY:%.*]] ], [ [[CMP3_I]], [[LAND_RHS_I]] ]
+; CHECK-NEXT:    ret i1 [[PHI]]
 ;
 entry:
   %v0 = load i32, ptr %a, align 4