Prevent Constant Folding From Optimizing inrange GEP

[zhaomo]

Summary

This patch does the following things:

1. update SymbolicallyEvaluateGEP so that it bails out if it cannot preserve inrange arribute;
2. update llvm/test/Analysis/ConstantFolding/gep.ll to remove UB in it;
3. remove inaccurate comment above ConstantFoldInstOperandsImpl in llvm/lib/Analysis/ConstantFolding.cpp;
4. add a new regression test that makes sure that no optimizations change an inrange GEP in an unexpected way.

[pcc]

llvm/lib/Analysis/ConstantFolding.cpp

991 ↗

(On Diff #164088)

I think this TODO is no longer accurate: we *are* being passed the instruction/constant in InstOrCE , so we end up preserving inbounds and inrange and could preserve nsw/nuw if we chose. I would remove it.

1008 ↗

(On Diff #164088)

Can this check be moved here?
http://llvm-cs.pcc.me.uk/lib/Analysis/ConstantFolding.cpp#957
I was imagining that if we fail to preserve inrange then we would return nullptr instead of dropping it.

[pcc]

Please update the commit message to reflect what this patch is now doing.

llvm/test/Analysis/ConstantFolding/gep.ll

14 ↗

(On Diff #164105)

Can you update the other tests to return the pointer instead of loading?

[pcc]

LGTM

llvm/trunk/lib/Analysis/ConstantFolding.cpp

Constant *SymbolicallyEvaluateGEP(const GEPOperator *GEP,
   // Preserve the inrange index from the innermost GEP if possible. We must
   // have calculated the same indices up to and including the inrange index.
   Optional<unsigned> InRangeIndex;
   if (Optional<unsigned> LastIRIndex = InnermostGEP->getInRangeIndex())
     if (SrcElemTy == InnermostGEP->getSourceElementType() &&
         NewIdxs.size() > *LastIRIndex) {
       InRangeIndex = LastIRIndex;
       for (unsigned I = 0; I <= *LastIRIndex; ++I)
-        if (NewIdxs[I] != InnermostGEP->getOperand(I + 1)) {
-          InRangeIndex = None;
-          break;
-        }
+        if (NewIdxs[I] != InnermostGEP->getOperand(I + 1))
+          return nullptr;
     }
 
   // Create a GEP.
   Constant *C = ConstantExpr::getGetElementPtr(SrcElemTy, Ptr, NewIdxs,
                                                InBounds, InRangeIndex);
   assert(C->getType()->getPointerElementType() == Ty &&
          "Computed GetElementPtr has unexpected type!");
 
   // If we ended up indexing a member with a type that doesn't match
   // the type of what the original indices indexed, add a cast.
   if (Ty != ResElemTy)
     C = FoldBitCast(C, ResTy, DL);
 
   return C;
 }
 
 /// Attempt to constant fold an instruction with the
 /// specified opcode and operands.  If successful, the constant result is
 /// returned, if not, null is returned.  Note that this function can fail when
 /// attempting to fold instructions like loads and stores, which have no
 /// constant expression form.
-///
-/// TODO: This function neither utilizes nor preserves nsw/nuw/inbounds/inrange
-/// etc information, due to only being passed an opcode and operands. Constant
-/// folding using this function strips this information.
-///
 Constant *ConstantFoldInstOperandsImpl(const Value *InstOrCE, unsigned Opcode,
                                        ArrayRef<Constant *> Ops,
                                        const DataLayout &DL,
                                        const TargetLibraryInfo *TLI) {
   Type *DestTy = InstOrCE->getType();
 
   // Handle easy binops first.
   if (Instruction::isBinaryOp(Opcode))

llvm/trunk/test/Analysis/ConstantFolding/gep.ll

 ; RUN: opt -instcombine -S -o - %s | FileCheck %s
 ; Tests that we preserve the inrange attribute on indices where possible.
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 %struct.A = type { i32 (...)** }
 
 @vt = external global [3 x i8*]
 
-; CHECK: define i32 (...)* @f0()
-define i32 (...)* @f0() {
-  ; CHECK-NEXT: load i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, inrange i64 0, i64 2) to i32 (...)**)
-  %load = load i32 (...)*, i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, inrange i64 0, i64 1) to i32 (...)**), i64 1)
-  ret i32 (...)* %load
+; CHECK: define i32 (...)** @f0()
+define i32 (...)** @f0() {
+  ; CHECK-NEXT: ret i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, inrange i64 0, i64 2) to i32 (...)**
+  ret i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, inrange i64 0, i64 1) to i32 (...)**), i64 1)
 }
 
-; CHECK: define i32 (...)* @f1()
-define i32 (...)* @f1() {
-  ; CHECK-NEXT: load i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, i64 0, i64 2) to i32 (...)**)
-  %load = load i32 (...)*, i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, i64 0, inrange i64 1) to i32 (...)**), i64 1)
-  ret i32 (...)* %load
+; CHECK: define i32 (...)** @f1()
+define i32 (...)** @f1() {
+  ; CHECK-NEXT: ret i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, i64 0, inrange i64 1) to i32 (...)**), i64 1)
+  ret i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, i64 0, inrange i64 1) to i32 (...)**), i64 1)
 }
 
-; CHECK: define i32 (...)* @f2()
-define i32 (...)* @f2() {
-  ; CHECK-NEXT: load i32 (...)*, i32 (...)** bitcast (i8** getelementptr ([3 x i8*], [3 x i8*]* @vt, i64 1, i64 1) to i32 (...)**)
-  %load = load i32 (...)*, i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, i64 0, inrange i64 1) to i32 (...)**), i64 3)
-  ret i32 (...)* %load
+; CHECK: define i32 (...)** @f2()
+define i32 (...)** @f2() {
+  ; CHECK-NEXT: ret i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, i64 0, inrange i64 1) to i32 (...)**), i64 3)
+  ret i32 (...)** getelementptr (i32 (...)*, i32 (...)** bitcast (i8** getelementptr inbounds ([3 x i8*], [3 x i8*]* @vt, i64 0, inrange i64 1) to i32 (...)**), i64 3)
 }

llvm/trunk/test/Other/optimize-inrange-gep.ll

+; RUN: opt -O0 -S < %s | FileCheck %s
+; RUN: opt -O1 -S < %s | FileCheck %s
+; RUN: opt -O2 -S < %s | FileCheck %s
+; RUN: opt -O3 -S < %s | FileCheck %s
+; RUN: opt -Os -S < %s | FileCheck %s
+; RUN: opt -Oz -S < %s | FileCheck %s
+
+target datalayout = "e-p:64:64"
+
+; Make sure that optimizations do not optimize inrange GEP.
+
+@vtable = constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* null, i8* null] }
+
+define void @foo(i8*** %p) {
+  ;CHECK: store i8** getelementptr {{.*}} ({ [3 x i8*] }, { [3 x i8*] }* @vtable, i{{.*}} 0, inrange i32 0, i{{.*}} 3), i8*** %p
+  store i8** getelementptr ({ [3 x i8*] }, { [3 x i8*] }* @vtable, i32 0, inrange i32 0, i32 3), i8*** %p
+  ret void
+}