From 26fe7ebc03c14a475689d8de6f696cdf59a66195 Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Mon, 14 Jan 2008 02:09:12 +0000 Subject: [PATCH] Fix the miscompilation of MiBench/consumer-lame that was exposed by Evan's byval work. This miscompilation is due to the program indexing an array out of range and us doing a transformation that broke this. llvm-svn: 45949 --- llvm/lib/Transforms/IPO/GlobalOpt.cpp | 172 +++++++++++------- .../GlobalOpt/2008-01-13-OutOfRangeSROA.ll | 16 ++ 2 files changed, 121 insertions(+), 67 deletions(-) create mode 100644 llvm/test/Transforms/GlobalOpt/2008-01-13-OutOfRangeSROA.ll diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp index 826f81e59320..ea8080e35e89 100644 --- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp +++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp @@ -26,6 +26,7 @@ #include "llvm/Target/TargetData.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/Debug.h" +#include "llvm/Support/GetElementPtrTypeIterator.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/Statistic.h" @@ -335,76 +336,113 @@ static bool CleanupConstantGlobalUsers(Value *V, Constant *Init) { return Changed; } - -/// UsersSafeToSRA - Look at all uses of the global and decide whether it is -/// safe for us to perform this transformation. -/// -static bool UsersSafeToSRA(Value *V) { - for (Value::use_iterator UI = V->use_begin(), E = V->use_end(); UI != E;++UI){ - if (ConstantExpr *CE = dyn_cast(*UI)) { - if (CE->getOpcode() != Instruction::GetElementPtr) - return false; - - // Check to see if this ConstantExpr GEP is SRA'able. In particular, we - // don't like < 3 operand CE's, and we don't like non-constant integer - // indices. - if (CE->getNumOperands() < 3 || !CE->getOperand(1)->isNullValue()) - return false; - - for (unsigned i = 1, e = CE->getNumOperands(); i != e; ++i) - if (!isa(CE->getOperand(i))) - return false; - - if (!UsersSafeToSRA(CE)) return false; - continue; - } - - if (Instruction *I = dyn_cast(*UI)) { - if (isa(I)) continue; - - if (StoreInst *SI = dyn_cast(I)) { - // Don't allow a store OF the address, only stores TO the address. - if (SI->getOperand(0) == V) return false; - continue; - } - - if (isa(I)) { - if (!UsersSafeToSRA(I)) return false; - - // If the first two indices are constants, this can be SRA'd. - if (isa(I->getOperand(0))) { - if (I->getNumOperands() < 3 || !isa(I->getOperand(1)) || - !cast(I->getOperand(1))->isNullValue() || - !isa(I->getOperand(2))) - return false; - continue; - } - - if (ConstantExpr *CE = dyn_cast(I->getOperand(0))){ - if (CE->getOpcode() != Instruction::GetElementPtr || - CE->getNumOperands() < 3 || I->getNumOperands() < 2 || - !isa(I->getOperand(0)) || - !cast(I->getOperand(0))->isNullValue()) - return false; - continue; - } - return false; - } - return false; // Any other instruction is not safe. - } - if (Constant *C = dyn_cast(*UI)) { - // We might have a dead and dangling constant hanging off of here. - if (!ConstantIsDead(C)) - return false; - continue; - } - // Otherwise must be some other user. - return false; - } +/// isSafeSROAElementUse - Return true if the specified instruction is a safe +/// user of a derived expression from a global that we want to SROA. +static bool isSafeSROAElementUse(Value *V) { + // We might have a dead and dangling constant hanging off of here. + if (Constant *C = dyn_cast(V)) + return ConstantIsDead(C); + Instruction *I = dyn_cast(V); + if (!I) return false; + + // Loads are ok. + if (isa(I)) return true; + + // Stores *to* the pointer are ok. + if (StoreInst *SI = dyn_cast(I)) + return SI->getOperand(0) != V; + + // Otherwise, it must be a GEP. + GetElementPtrInst *GEPI = dyn_cast(I); + if (GEPI == 0) return false; + + if (GEPI->getNumOperands() < 3 || !isa(GEPI->getOperand(1)) || + !cast(GEPI->getOperand(1))->isNullValue()) + return false; + + for (Value::use_iterator I = GEPI->use_begin(), E = GEPI->use_end(); + I != E; ++I) + if (!isSafeSROAElementUse(*I)) + return false; return true; } + +/// IsUserOfGlobalSafeForSRA - U is a direct user of the specified global value. +/// Look at it and its uses and decide whether it is safe to SROA this global. +/// +static bool IsUserOfGlobalSafeForSRA(User *U, GlobalValue *GV) { + // The user of the global must be a GEP Inst or a ConstantExpr GEP. + if (!isa(U) && + (!isa(U) || + cast(U)->getOpcode() != Instruction::GetElementPtr)) + return false; + + // Check to see if this ConstantExpr GEP is SRA'able. In particular, we + // don't like < 3 operand CE's, and we don't like non-constant integer + // indices. This enforces that all uses are 'gep GV, 0, C, ...' for some + // value of C. + if (U->getNumOperands() < 3 || !isa(U->getOperand(1)) || + !cast(U->getOperand(1))->isNullValue() || + !isa(U->getOperand(2))) + return false; + + gep_type_iterator GEPI = gep_type_begin(U), E = gep_type_end(U); + ++GEPI; // Skip over the pointer index. + + // If this is a use of an array allocation, do a bit more checking for sanity. + if (const ArrayType *AT = dyn_cast(*GEPI)) { + uint64_t NumElements = AT->getNumElements(); + ConstantInt *Idx = cast(U->getOperand(2)); + + // Check to make sure that index falls within the array. If not, + // something funny is going on, so we won't do the optimization. + // + if (Idx->getZExtValue() >= NumElements) + return false; + + // We cannot scalar repl this level of the array unless any array + // sub-indices are in-range constants. In particular, consider: + // A[0][i]. We cannot know that the user isn't doing invalid things like + // allowing i to index an out-of-range subscript that accesses A[1]. + // + // Scalar replacing *just* the outer index of the array is probably not + // going to be a win anyway, so just give up. + for (++GEPI; // Skip array index. + GEPI != E && (isa(*GEPI) || isa(*GEPI)); + ++GEPI) { + uint64_t NumElements; + if (const ArrayType *SubArrayTy = dyn_cast(*GEPI)) + NumElements = SubArrayTy->getNumElements(); + else + NumElements = cast(*GEPI)->getNumElements(); + + ConstantInt *IdxVal = dyn_cast(GEPI.getOperand()); + if (!IdxVal || IdxVal->getZExtValue() >= NumElements) + return false; + } + } + + for (Value::use_iterator I = U->use_begin(), E = U->use_end(); I != E; ++I) + if (!isSafeSROAElementUse(*I)) + return false; + return true; +} + +/// GlobalUsersSafeToSRA - Look at all uses of the global and decide whether it +/// is safe for us to perform this transformation. +/// +static bool GlobalUsersSafeToSRA(GlobalValue *GV) { + for (Value::use_iterator UI = GV->use_begin(), E = GV->use_end(); + UI != E; ++UI) { + if (!IsUserOfGlobalSafeForSRA(*UI, GV)) + return false; + } + return true; +} + + /// SRAGlobal - Perform scalar replacement of aggregates on the specified global /// variable. This opens the door for other optimizations by exposing the /// behavior of the program in a more fine-grained way. We have determined that @@ -412,7 +450,7 @@ static bool UsersSafeToSRA(Value *V) { /// insert so that the caller can reprocess it. static GlobalVariable *SRAGlobal(GlobalVariable *GV) { // Make sure this global only has simple uses that we can SRA. - if (!UsersSafeToSRA(GV)) + if (!GlobalUsersSafeToSRA(GV)) return 0; assert(GV->hasInternalLinkage() && !GV->isConstant()); diff --git a/llvm/test/Transforms/GlobalOpt/2008-01-13-OutOfRangeSROA.ll b/llvm/test/Transforms/GlobalOpt/2008-01-13-OutOfRangeSROA.ll new file mode 100644 index 000000000000..aace34654976 --- /dev/null +++ b/llvm/test/Transforms/GlobalOpt/2008-01-13-OutOfRangeSROA.ll @@ -0,0 +1,16 @@ +; RUN: llvm-as < %s | opt -globalopt | llvm-dis | grep {16 x .31 x double.. zeroinitializer} + +; The 'X' indices could be larger than 31. Do not SROA the outer indices of this array. +@mm = internal global [16 x [31 x double]] zeroinitializer, align 32 + +define void @test(i32 %X) { + %P = getelementptr [16 x [31 x double]]* @mm, i32 0, i32 0, i32 %X + store double 1.0, double* %P + ret void +} + +define double @get(i32 %X) { + %P = getelementptr [16 x [31 x double]]* @mm, i32 0, i32 0, i32 %X + %V = load double* %P + ret double %V +}