From a826a887550a4fc706516a9d8861acd5a5efd693 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 11 Nov 2010 21:23:25 +0000 Subject: [PATCH] Factor out Instruction::isSafeToSpeculativelyExecute's code for testing for dereferenceable pointers into a helper function, isDereferenceablePointer. Teach it how to reason about GEPs with simple non-zero indices. Also eliminate ArgumentPromtion's IsAlwaysValidPointer, which didn't check for weak externals or out of range gep indices. llvm-svn: 118840 --- llvm/include/llvm/Value.h | 4 + llvm/lib/Transforms/IPO/ArgumentPromotion.cpp | 15 +-- llvm/lib/VMCore/Instruction.cpp | 21 +---- llvm/lib/VMCore/Value.cpp | 55 +++++++++++ .../SimplifyCFG/speculate-with-offset.ll | 94 +++++++++++++++++++ 5 files changed, 157 insertions(+), 32 deletions(-) create mode 100644 llvm/test/Transforms/SimplifyCFG/speculate-with-offset.ll diff --git a/llvm/include/llvm/Value.h b/llvm/include/llvm/Value.h index 8740f353ab51..8dc4105b9e4c 100644 --- a/llvm/include/llvm/Value.h +++ b/llvm/include/llvm/Value.h @@ -294,6 +294,10 @@ public: const Value *getUnderlyingObject(unsigned MaxLookup = 6) const { return const_cast(this)->getUnderlyingObject(MaxLookup); } + + /// isDereferenceablePointer - Test if this value is always a pointer to + /// allocated and suitably aligned memory for a simple load or store. + bool isDereferenceablePointer() const; /// DoPHITranslation - If this value is a PHI node with CurBB as its parent, /// return the value in the PHI node corresponding to PredBB. If not, return diff --git a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp index 78d9f1816e48..fe30381170dc 100644 --- a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp +++ b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp @@ -188,19 +188,6 @@ CallGraphNode *ArgPromotion::PromoteArguments(CallGraphNode *CGN) { return DoPromotion(F, ArgsToPromote, ByValArgsToTransform); } -/// IsAlwaysValidPointer - Return true if the specified pointer is always legal -/// to load. -static bool IsAlwaysValidPointer(Value *V) { - if (isa(V) || isa(V)) return true; - if (GetElementPtrInst *GEP = dyn_cast(V)) - return IsAlwaysValidPointer(GEP->getOperand(0)); - if (ConstantExpr *CE = dyn_cast(V)) - if (CE->getOpcode() == Instruction::GetElementPtr) - return IsAlwaysValidPointer(CE->getOperand(0)); - - return false; -} - /// AllCalleesPassInValidPointerForArgument - Return true if we can prove that /// all callees pass in a valid pointer for the specified function argument. static bool AllCalleesPassInValidPointerForArgument(Argument *Arg) { @@ -216,7 +203,7 @@ static bool AllCalleesPassInValidPointerForArgument(Argument *Arg) { CallSite CS(*UI); assert(CS && "Should only have direct calls!"); - if (!IsAlwaysValidPointer(CS.getArgument(ArgNo))) + if (!CS.getArgument(ArgNo)->isDereferenceablePointer()) return false; } return true; diff --git a/llvm/lib/VMCore/Instruction.cpp b/llvm/lib/VMCore/Instruction.cpp index 05bed4c64316..fdddba988c3d 100644 --- a/llvm/lib/VMCore/Instruction.cpp +++ b/llvm/lib/VMCore/Instruction.cpp @@ -398,25 +398,10 @@ bool Instruction::isSafeToSpeculativelyExecute() const { return Op && !Op->isNullValue() && !Op->isAllOnesValue(); } case Load: { - if (cast(this)->isVolatile()) + const LoadInst *LI = cast(this); + if (LI->isVolatile()) return false; - // Note that it is not safe to speculate into a malloc'd region because - // malloc may return null. - // It's also not safe to follow a bitcast, for example: - // bitcast i8* (alloca i8) to i32* - // would result in a 4-byte load from a 1-byte alloca. - Value *Op0 = getOperand(0); - if (GEPOperator *GEP = dyn_cast(Op0)) { - // TODO: it's safe to do this for any GEP with constant indices that - // compute inside the allocated type, but not for any inbounds gep. - if (GEP->hasAllZeroIndices()) - Op0 = GEP->getPointerOperand(); - } - if (isa(Op0)) - return true; - if (GlobalVariable *GV = dyn_cast(getOperand(0))) - return !GV->hasExternalWeakLinkage(); - return false; + return LI->getPointerOperand()->isDereferenceablePointer(); } case Call: return false; // The called function could have undefined behavior or diff --git a/llvm/lib/VMCore/Value.cpp b/llvm/lib/VMCore/Value.cpp index b8c677565467..d5e39979608d 100644 --- a/llvm/lib/VMCore/Value.cpp +++ b/llvm/lib/VMCore/Value.cpp @@ -22,6 +22,7 @@ #include "llvm/ValueSymbolTable.h" #include "llvm/ADT/SmallString.h" #include "llvm/Support/Debug.h" +#include "llvm/Support/GetElementPtrTypeIterator.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/LeakDetector.h" #include "llvm/Support/ManagedStatic.h" @@ -366,6 +367,60 @@ Value *Value::getUnderlyingObject(unsigned MaxLookup) { return V; } +/// isDereferenceablePointer - Test if this value is always a pointer to +// allocated and suitably aligned memory for a simple load or store. +bool Value::isDereferenceablePointer() const { + // Note that it is not safe to speculate into a malloc'd region because + // malloc may return null. + // It's also not always safe to follow a bitcast, for example: + // bitcast i8* (alloca i8) to i32* + // would result in a 4-byte load from a 1-byte alloca. Some cases could + // be handled using TargetData to check sizes and alignments though. + + // These are obviously ok. + if (isa(this)) return true; + + // Global variables which can't collapse to null are ok. + if (const GlobalVariable *GV = dyn_cast(this)) + return !GV->hasExternalWeakLinkage(); + + // For GEPs, determine if the indexing lands within the allocated object. + if (const GEPOperator *GEP = dyn_cast(this)) { + // Conservatively require that the base pointer be fully dereferenceable. + if (!GEP->getOperand(0)->isDereferenceablePointer()) + return false; + // Check the indices. + gep_type_iterator GTI = gep_type_begin(GEP); + for (User::const_op_iterator I = GEP->op_begin()+1, + E = GEP->op_end(); I != E; ++I) { + Value *Index = *I; + const Type *Ty = *GTI++; + // Struct indices can't be out of bounds. + if (isa(Ty)) + continue; + ConstantInt *CI = dyn_cast(Index); + if (!CI) + return false; + // Zero is always ok. + if (CI->isZero()) + continue; + // Check to see that it's within the bounds of an array. + const ArrayType *ATy = dyn_cast(Ty); + if (!ATy) + return false; + if (CI->getValue().getActiveBits() > 64) + return false; + if (CI->getZExtValue() >= ATy->getNumElements()) + return false; + } + // Indices check out; this is dereferenceable. + return true; + } + + // If we don't know, assume the worst. + return false; +} + /// DoPHITranslation - If this value is a PHI node with CurBB as its parent, /// return the value in the PHI node corresponding to PredBB. If not, return /// ourself. This is useful if you want to know the value something has in a diff --git a/llvm/test/Transforms/SimplifyCFG/speculate-with-offset.ll b/llvm/test/Transforms/SimplifyCFG/speculate-with-offset.ll new file mode 100644 index 000000000000..a737d5602e84 --- /dev/null +++ b/llvm/test/Transforms/SimplifyCFG/speculate-with-offset.ll @@ -0,0 +1,94 @@ +; RUN: opt -simplifycfg -S < %s | FileCheck %s + +; This load is safe to speculate, as it's from a safe offset +; within an alloca. + +; CHECK: @yes +; CHECK-NOT: br + +define void @yes(i1 %c) nounwind { +entry: + %a = alloca [4 x i64*], align 8 + %__a.addr = getelementptr [4 x i64*]* %a, i64 0, i64 3 + call void @frob(i64** %__a.addr) + br i1 %c, label %if.then, label %if.end + +if.then: ; preds = %entry + br label %return + +if.end: ; preds = %entry + %tmp5 = load i64** %__a.addr, align 8 + br label %return + +return: ; preds = %if.end, %if.then + %storemerge = phi i64* [ undef, %if.then ], [ %tmp5, %if.end ] + ret void +} + +; CHECK: @no0 +; CHECK: br i1 %c + +define void @no0(i1 %c) nounwind { +entry: + %a = alloca [4 x i64*], align 8 + %__a.addr = getelementptr [4 x i64*]* %a, i64 0, i64 4 + call void @frob(i64** %__a.addr) + br i1 %c, label %if.then, label %if.end + +if.then: ; preds = %entry + br label %return + +if.end: ; preds = %entry + %tmp5 = load i64** %__a.addr, align 8 + br label %return + +return: ; preds = %if.end, %if.then + %storemerge = phi i64* [ undef, %if.then ], [ %tmp5, %if.end ] + ret void +} + +; CHECK: @no1 +; CHECK: br i1 %c + +define void @no1(i1 %c, i64 %n) nounwind { +entry: + %a = alloca [4 x i64*], align 8 + %__a.addr = getelementptr [4 x i64*]* %a, i64 0, i64 %n + call void @frob(i64** %__a.addr) + br i1 %c, label %if.then, label %if.end + +if.then: ; preds = %entry + br label %return + +if.end: ; preds = %entry + %tmp5 = load i64** %__a.addr, align 8 + br label %return + +return: ; preds = %if.end, %if.then + %storemerge = phi i64* [ undef, %if.then ], [ %tmp5, %if.end ] + ret void +} + +; CHECK: @no2 +; CHECK: br i1 %c + +define void @no2(i1 %c, i64 %n) nounwind { +entry: + %a = alloca [4 x i64*], align 8 + %__a.addr = getelementptr [4 x i64*]* %a, i64 1, i64 0 + call void @frob(i64** %__a.addr) + br i1 %c, label %if.then, label %if.end + +if.then: ; preds = %entry + br label %return + +if.end: ; preds = %entry + %tmp5 = load i64** %__a.addr, align 8 + br label %return + +return: ; preds = %if.end, %if.then + %storemerge = phi i64* [ undef, %if.then ], [ %tmp5, %if.end ] + ret void +} + +declare void @frob(i64** nocapture %p)