[InstCombine] Preserve inbounds when merging with zero-index GEP (PR44423)

[nikic]

Summary

This addresses https://bugs.llvm.org/show_bug.cgi?id=44423. If one of the GEPs is inbounds and the other is zero-index, we can also preserve inbounds.

[davezarzycki]

Bisecting identifies this change as breaking stage two testing on my Fedora 31 machine. Can we revert this?

FAIL: LLVM :: Transforms/Attributor/liveness.ll (44058 of 60349)
******************** TEST 'LLVM :: Transforms/Attributor/liveness.ll' FAILED ********************
Script:
--
: 'RUN: at line 2';   /tmp/_update_lc/t/bin/opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=4 -S < /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll --check-prefixes=CHECK,OLDPM
: 'RUN: at line 3';   /tmp/_update_lc/t/bin/opt -passes=attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=4 -S < /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll --check-prefixes=CHECK,NEWPM
--
Exit Code: 2

Command Output (stderr):
--
opt: /home/dave/s/lp/llvm/lib/Transforms/IPO/Attributor.cpp:6475: llvm::ChangeStatus llvm::Attributor::rewriteFunctionSignatures(): Assertion `OldFn->getNumUses() == 0 && "Unexpected leftover uses!"' failed.
Stack dump:
0.  Program arguments: /tmp/_update_lc/t/bin/opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=4 -S
1.  Running pass 'Deduce and propagate attributes' on module '<stdin>'.
 #0 0x00000000023d387f llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/tmp/_update_lc/t/bin/opt+0x23d387f)
 #1 0x00000000023d1ad2 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/bin/opt+0x23d1ad2)
 #2 0x00000000023d3e05 SignalHandler(int) (/tmp/_update_lc/t/bin/opt+0x23d3e05)
 #3 0x00007ffff7fa8b20 __restore_rt (/lib64/libpthread.so.0+0x14b20)
 #4 0x00007ffff616d625 raise (/lib64/libc.so.6+0x3c625)
 #5 0x00007ffff61568d9 abort (/lib64/libc.so.6+0x258d9)
 #6 0x00007ffff61567a9 _nl_load_domain.cold (/lib64/libc.so.6+0x257a9)
 #7 0x00007ffff6165a66 (/lib64/libc.so.6+0x34a66)
 #8 0x0000000001dc276c llvm::Attributor::rewriteFunctionSignatures() (/tmp/_update_lc/t/bin/opt+0x1dc276c)
 #9 0x0000000001dc0c60 llvm::Attributor::run(llvm::Module&) (/tmp/_update_lc/t/bin/opt+0x1dc0c60)
#10 0x0000000001dc6e5f runAttributorOnModule(llvm::Module&, llvm::AnalysisGetter&) (/tmp/_update_lc/t/bin/opt+0x1dc6e5f)
#11 0x0000000001dca1fb (anonymous namespace)::AttributorLegacyPass::runOnModule(llvm::Module&) (/tmp/_update_lc/t/bin/opt+0x1dca1fb)
#12 0x0000000001d06a63 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/tmp/_update_lc/t/bin/opt+0x1d06a63)
#13 0x0000000001374e67 main (/tmp/_update_lc/t/bin/opt+0x1374e67)
#14 0x00007ffff61581a3 __libc_start_main (/lib64/libc.so.6+0x271a3)
#15 0x000000000135f7ee _start (/tmp/_update_lc/t/bin/opt+0x135f7ee)
FileCheck error: '-' is empty.
FileCheck command line:  /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll --check-prefixes=CHECK,OLDPM

[lebedev.ri]

In D72060#1801410, @davezarzycki wrote:

Bisecting identifies this change as breaking stage two testing on my Fedora 31 machine. Can we revert this?

FAIL: LLVM :: Transforms/Attributor/liveness.ll (44058 of 60349)
******************** TEST 'LLVM :: Transforms/Attributor/liveness.ll' FAILED ********************
Script:
--
: 'RUN: at line 2';   /tmp/_update_lc/t/bin/opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=4 -S < /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll --check-prefixes=CHECK,OLDPM
: 'RUN: at line 3';   /tmp/_update_lc/t/bin/opt -passes=attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=4 -S < /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll | /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll --check-prefixes=CHECK,NEWPM
--
Exit Code: 2

Command Output (stderr):
--
opt: /home/dave/s/lp/llvm/lib/Transforms/IPO/Attributor.cpp:6475: llvm::ChangeStatus llvm::Attributor::rewriteFunctionSignatures(): Assertion `OldFn->getNumUses() == 0 && "Unexpected leftover uses!"' failed.
Stack dump:
0.  Program arguments: /tmp/_update_lc/t/bin/opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=4 -S
1.  Running pass 'Deduce and propagate attributes' on module '<stdin>'.
 #0 0x00000000023d387f llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/tmp/_update_lc/t/bin/opt+0x23d387f)
 #1 0x00000000023d1ad2 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/bin/opt+0x23d1ad2)
 #2 0x00000000023d3e05 SignalHandler(int) (/tmp/_update_lc/t/bin/opt+0x23d3e05)
 #3 0x00007ffff7fa8b20 __restore_rt (/lib64/libpthread.so.0+0x14b20)
 #4 0x00007ffff616d625 raise (/lib64/libc.so.6+0x3c625)
 #5 0x00007ffff61568d9 abort (/lib64/libc.so.6+0x258d9)
 #6 0x00007ffff61567a9 _nl_load_domain.cold (/lib64/libc.so.6+0x257a9)
 #7 0x00007ffff6165a66 (/lib64/libc.so.6+0x34a66)
 #8 0x0000000001dc276c llvm::Attributor::rewriteFunctionSignatures() (/tmp/_update_lc/t/bin/opt+0x1dc276c)
 #9 0x0000000001dc0c60 llvm::Attributor::run(llvm::Module&) (/tmp/_update_lc/t/bin/opt+0x1dc0c60)
#10 0x0000000001dc6e5f runAttributorOnModule(llvm::Module&, llvm::AnalysisGetter&) (/tmp/_update_lc/t/bin/opt+0x1dc6e5f)
#11 0x0000000001dca1fb (anonymous namespace)::AttributorLegacyPass::runOnModule(llvm::Module&) (/tmp/_update_lc/t/bin/opt+0x1dca1fb)
#12 0x0000000001d06a63 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/tmp/_update_lc/t/bin/opt+0x1d06a63)
#13 0x0000000001374e67 main (/tmp/_update_lc/t/bin/opt+0x1374e67)
#14 0x00007ffff61581a3 __libc_start_main (/lib64/libc.so.6+0x271a3)
#15 0x000000000135f7ee _start (/tmp/_update_lc/t/bin/opt+0x135f7ee)
FileCheck error: '-' is empty.
FileCheck command line:  /tmp/_update_lc/t/bin/FileCheck /home/dave/s/lp/llvm/test/Transforms/Attributor/liveness.ll --check-prefixes=CHECK,OLDPM

Is there a reproducer in progress?

[davezarzycki]

In D72060#1801426, @lebedev.ri wrote:

Is there a reproducer in progress?

Never mind. This may or may not be the root cause, but I can't be sure due to an unrelated race condition in LLVM's thread pool logic making my bisects unreliable (see D72090 for the fix). I'm re-bisecting with the patch from D72090.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

if (auto *NewBinOp = dyn_cast<BinaryOperator>(NarrowBO)) {
     if (IsSext)
       NewBinOp->setHasNoSignedWrap();
     else
       NewBinOp->setHasNoUnsignedWrap();
   }
   return CastInst::Create(CastOpc, NarrowBO, BO.getType());
 }
 
+bool isMergedGEPInBounds(GEPOperator &GEP1, GEPOperator &GEP2) {
+  // At least one GEP must be inbounds.
+  if (!GEP1.isInBounds() && !GEP2.isInBounds())
+    return false;
+
+  return (GEP1.isInBounds() || GEP1.hasAllZeroIndices()) &&
+         (GEP2.isInBounds() || GEP2.hasAllZeroIndices());
+}
+
 Instruction *InstCombiner::visitGetElementPtrInst(GetElementPtrInst &GEP) {
   SmallVector<Value*, 8> Ops(GEP.op_begin(), GEP.op_end());
   Type *GEPType = GEP.getType();
   Type *GEPEltType = GEP.getSourceElementType();
   if (Value *V = SimplifyGEPInst(GEPEltType, Ops, SQ.getWithInstruction(&GEP)))
     return replaceInstUsesWith(GEP, V);
 
   // For vector geps, use the generic demanded vector support.
if (EndsWithSequential) {
           SimplifyAddInst(GO1, SO1, false, false, SQ.getWithInstruction(&GEP));
       // Only do the combine when we are sure the cost after the
       // merge is never more than that before the merge.
       if (Sum == nullptr)
         return nullptr;
 
       // Update the GEP in place if possible.
       if (Src->getNumOperands() == 2) {
-        GEP.setIsInBounds(GEP.isInBounds() && Src->isInBounds());
+        GEP.setIsInBounds(isMergedGEPInBounds(*Src, *cast<GEPOperator>(&GEP)));
         GEP.setOperand(0, Src->getOperand(0));
         GEP.setOperand(1, Sum);
         return &GEP;
       }
       Indices.append(Src->op_begin()+1, Src->op_end()-1);
       Indices.push_back(Sum);
       Indices.append(GEP.op_begin()+2, GEP.op_end());
     } else if (isa<Constant>(*GEP.idx_begin()) &&
                cast<Constant>(*GEP.idx_begin())->isNullValue() &&
                Src->getNumOperands() != 1) {
       // Otherwise we can do the fold if the first index of the GEP is a zero
       Indices.append(Src->op_begin()+1, Src->op_end());
       Indices.append(GEP.idx_begin()+1, GEP.idx_end());
     }
 
     if (!Indices.empty())
-      return GEP.isInBounds() && Src->isInBounds()
+      return isMergedGEPInBounds(*Src, *cast<GEPOperator>(&GEP))
                  ? GetElementPtrInst::CreateInBounds(
                        Src->getSourceElementType(), Src->getOperand(0), Indices,
                        GEP.getName())
                  : GetElementPtrInst::Create(Src->getSourceElementType(),
                                              Src->getOperand(0), Indices,
                                              GEP.getName());
   }
 

llvm/test/Transforms/InstCombine/getelementptr.ll

 ;
   %tmp0 = bitcast i32** %ptr to i32*
   %tmp1 = getelementptr inbounds i32, i32* %tmp0, <2 x i64> <i64 0, i64 1>
   ret <2 x i32*> %tmp1
 }
 
 define i32* @test_bitcast_nzgep([1 x i32]* %base, i64 %idx) {
 ; CHECK-LABEL: @test_bitcast_nzgep(
-; CHECK-NEXT:    [[PTR:%.*]] = getelementptr [1 x i32], [1 x i32]* [[BASE:%.*]], i64 0, i64 [[IDX:%.*]]
+; CHECK-NEXT:    [[PTR:%.*]] = getelementptr inbounds [1 x i32], [1 x i32]* [[BASE:%.*]], i64 0, i64 [[IDX:%.*]]
 ; CHECK-NEXT:    ret i32* [[PTR]]
 ;
   %base2 = bitcast [1 x i32]* %base to i32*
   %ptr = getelementptr inbounds i32, i32* %base2, i64 %idx
   ret i32* %ptr
 }
 
 define i32* @test_zgep_nzgep([1 x i32]* %base, i64 %idx) {
 ; CHECK-LABEL: @test_zgep_nzgep(
-; CHECK-NEXT:    [[PTR:%.*]] = getelementptr [1 x i32], [1 x i32]* [[BASE:%.*]], i64 0, i64 [[IDX:%.*]]
+; CHECK-NEXT:    [[PTR:%.*]] = getelementptr inbounds [1 x i32], [1 x i32]* [[BASE:%.*]], i64 0, i64 [[IDX:%.*]]
 ; CHECK-NEXT:    ret i32* [[PTR]]
 ;
   %base2 = getelementptr [1 x i32], [1 x i32]* %base, i64 0, i64 0
   %ptr = getelementptr inbounds i32, i32* %base2, i64 %idx
   ret i32* %ptr
 }
 
 define i32* @test_nzgep_zgep([1 x i32]* %base, i64 %idx) {
 ; CHECK-LABEL: @test_nzgep_zgep(
-; CHECK-NEXT:    [[PTR:%.*]] = getelementptr [1 x i32], [1 x i32]* [[BASE:%.*]], i64 [[IDX:%.*]], i64 0
+; CHECK-NEXT:    [[PTR:%.*]] = getelementptr inbounds [1 x i32], [1 x i32]* [[BASE:%.*]], i64 [[IDX:%.*]], i64 0
 ; CHECK-NEXT:    ret i32* [[PTR]]
 ;
   %base2 = getelementptr inbounds [1 x i32], [1 x i32]* %base, i64 %idx
   %ptr = getelementptr [1 x i32], [1 x i32]* %base2, i64 0, i64 0
   ret i32* %ptr
 }
 
 define i32* @test_gep_inbounds_of_gep(i32* %base) {
 ; CHECK-LABEL: @test_gep_inbounds_of_gep(
 ; CHECK-NEXT:    [[PTR2:%.*]] = getelementptr i32, i32* [[BASE:%.*]], i64 8
 ; CHECK-NEXT:    ret i32* [[PTR2]]
 ;
   %ptr1 = getelementptr i32, i32* %base, i64 4
   %ptr2 = getelementptr inbounds i32, i32* %ptr1, i64 4
   ret i32* %ptr2
 }