[MergeICmps] Do not perform the transformation if GEP is used outside of block

[twoh]

Summary

This patch prevents MergeICmps to performn the transformation if the address operand GEP of the load instruction has a use outside of the load's parent block. Without this patch, compiler crashes with the given test case because the use of %first.i is still around when the basic block is erased from https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Scalar/MergeICmps.cpp#L620. I think checking isUsedOutsideOfBlock with GEP is the original intention of the code, as the checking for LoadI is already performed in the same function.

This patch is incomplete though, as this makes the pass overly conservative and fails the test tuple-four-int8.ll. I believe what needs to be done is checking if GEP has a use outside of block that is not the part of "Comparisons" chain. Submit the patch as of now to prevent compiler crash.

[courbet]

I think checking isUsedOutsideOfBlock with GEP is the original intention of the code, as the checking for LoadI is already performed in the same function.

Indeed, this is a typo.

Thanks a lot.

test/Transforms/MergeICmps/X86/gep-used-outside.ll

6 ↗

(On Diff #172544)

please update the comment.

[twoh]

Comments for the test updated. Thanks for the comment!

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

if (auto *const LoadI = dyn_cast<LoadInst>(Val)) {
     // Do not optimize atomic loads to non-atomic memcmp
     if (!LoadI->isSimple()) {
       LLVM_DEBUG(dbgs() << "volatile or atomic\n");
       return {};
     }
     Value *const Addr = LoadI->getOperand(0);
     if (auto *const GEP = dyn_cast<GetElementPtrInst>(Addr)) {
       LLVM_DEBUG(dbgs() << "GEP\n");
-      if (LoadI->isUsedOutsideOfBlock(LoadI->getParent())) {
+      if (GEP->isUsedOutsideOfBlock(LoadI->getParent())) {
         LLVM_DEBUG(dbgs() << "used outside of block\n");
         return {};
       }
       const auto &DL = GEP->getModule()->getDataLayout();
       if (!isDereferenceablePointer(GEP, 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.

llvm/trunk/test/Transforms/MergeICmps/X86/gep-used-outside.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -mergeicmps -mtriple=x86_64-unknown-unknown -S | FileCheck %s
+
+%"struct.std::pair" = type { i32, i32 }
+
+; Check that the transformation is avoided when GEP has a use outside of the
+; parant block of the load instruction.
+
+define zeroext i32 @opeq1(
+; CHECK-LABEL: @opeq1(
+; CHECK-NOT:    [[MEMCMP:%.*]] = call i32 @memcmp
+
+  %"struct.std::pair"* nocapture readonly dereferenceable(16) %a,
+  %"struct.std::pair"* nocapture readonly dereferenceable(16) %b) local_unnamed_addr #0 {
+entry:
+  %first.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %a, i64 0, i32 1 
+  %0 = load i32, i32* %first.i, align 4
+  %first1.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %b, i64 0, i32 1 
+  %1 = load i32, i32* %first1.i, align 4
+  %cmp.i = icmp eq i32 %0, %1
+  br i1 %cmp.i, label %land.rhs.i, label %opeq1.exit
+
+land.rhs.i:
+  %second.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %a, i64 0, i32 0
+  %2 = load i32, i32* %second.i, align 4
+  %second2.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %b, i64 0, i32 0
+  %3 = load i32, i32* %second2.i, align 4
+  %cmp3.i = icmp eq i32 %2, %3
+  br label %opeq1.exit
+
+opeq1.exit:
+  %4 = phi i1 [ false, %entry ], [ %cmp3.i,  %land.rhs.i]
+  %5 = load i32, i32* %first.i, align 4
+  %6 = select i1 %4, i32 %5, i32 0
+  ret i32 %6
+}

llvm/trunk/test/Transforms/MergeICmps/X86/tuple-four-int8.ll

+; XFAIL: *
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -mergeicmps -mtriple=x86_64-unknown-unknown -S | FileCheck %s
 
 ; This is a more involved test: clang generates this weird pattern for
 ; tuple<uint8_t, uint8_t, uint8_t, uint8_t>. Right now we skip the entry block
 ; (which defines the base pointer for other blocks) and the last one (which
 ; does not have the expected structure). Only middle blocks (bytes [1,2]) are
 ; merged.