From 85d3d4f35de33c6d0d3800bd87619c00d06a98e2 Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Tue, 16 Dec 2008 21:24:51 +0000 Subject: [PATCH] Fix another crash found by inspection. If we have a PHI node merging the load multiple times, make sure the check the uses of the PHI to ensure they are transformable. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@61102 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/IPO/GlobalOpt.cpp | 115 +++++++++++------- .../GlobalOpt/2008-12-16-HeapSRACrash-2.ll | 24 ++++ 2 files changed, 92 insertions(+), 47 deletions(-) create mode 100644 test/Transforms/GlobalOpt/2008-12-16-HeapSRACrash-2.ll diff --git a/lib/Transforms/IPO/GlobalOpt.cpp b/lib/Transforms/IPO/GlobalOpt.cpp index ac8ff115fae..31830c6739e 100644 --- a/lib/Transforms/IPO/GlobalOpt.cpp +++ b/lib/Transforms/IPO/GlobalOpt.cpp @@ -1012,56 +1012,77 @@ static void ReplaceUsesOfMallocWithGlobal(Instruction *Alloc, } } -/// GlobalLoadUsesSimpleEnoughForHeapSRA - If all users of values loaded from -/// GV are simple enough to perform HeapSRA, return true. -static bool GlobalLoadUsesSimpleEnoughForHeapSRA(GlobalVariable *GV, - MallocInst *MI) { - for (Value::use_iterator UI = GV->use_begin(), E = GV->use_end(); UI != E; - ++UI) - if (LoadInst *LI = dyn_cast(*UI)) { - // We permit two users of the load: setcc comparing against the null - // pointer, and a getelementptr of a specific form. - for (Value::use_iterator UI = LI->use_begin(), E = LI->use_end(); - UI != E; ++UI) { - // Comparison against null is ok. - if (ICmpInst *ICI = dyn_cast(*UI)) { - if (!isa(ICI->getOperand(1))) - return false; - continue; - } +/// LoadUsesSimpleEnoughForHeapSRA - Verify that all uses of V (a load, or a phi +/// of a load) are simple enough to perform heap SRA on. This permits GEP's +/// that index through the array and struct field, icmps of null, and PHIs. +static bool LoadUsesSimpleEnoughForHeapSRA(Value *V, MallocInst *MI, + SmallPtrSet &AnalyzedLoadUsingPHIs) { + // We permit two users of the load: setcc comparing against the null + // pointer, and a getelementptr of a specific form. + for (Value::use_iterator UI = V->use_begin(), E = V->use_end(); UI != E;++UI){ + Instruction *User = cast(*UI); + + // Comparison against null is ok. + if (ICmpInst *ICI = dyn_cast(User)) { + if (!isa(ICI->getOperand(1))) + return false; + continue; + } + + // getelementptr is also ok, but only a simple form. + if (GetElementPtrInst *GEPI = dyn_cast(User)) { + // Must index into the array and into the struct. + if (GEPI->getNumOperands() < 3) + return false; + + // Otherwise the GEP is ok. + continue; + } + + if (PHINode *PN = dyn_cast(User)) { + // If we have already recursively analyzed this PHI, then it is safe. + if (AnalyzedLoadUsingPHIs.insert(PN)) + continue; + + // We have a phi of a load from the global. We can only handle this + // if the other PHI'd values are actually the same. In this case, + // the rewriter will just drop the phi entirely. + for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) { + Value *IV = PN->getIncomingValue(i); + if (IV == V) continue; // Trivial the same. - // getelementptr is also ok, but only a simple form. - if (GetElementPtrInst *GEPI = dyn_cast(*UI)) { - // Must index into the array and into the struct. - if (GEPI->getNumOperands() < 3) - return false; - - // Otherwise the GEP is ok. - continue; - } + // If the phi'd value is from the malloc that initializes the value, + // we can xform it. + if (IV == MI) continue; - if (PHINode *PN = dyn_cast(*UI)) { - // We have a phi of a load from the global. We can only handle this - // if the other PHI'd values are actually the same. In this case, - // the rewriter will just drop the phi entirely. - for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) { - Value *IV = PN->getIncomingValue(i); - if (IV == LI) continue; // Trivial the same. - - // If the phi'd value is from the malloc that initializes the value, - // we can xform it. - if (IV == MI) continue; - - // Otherwise, we don't know what it is. - return false; - } - continue; - } - - // Otherwise we don't know what this is, not ok. + // Otherwise, we don't know what it is. return false; } + + if (!LoadUsesSimpleEnoughForHeapSRA(PN, MI, AnalyzedLoadUsingPHIs)) + return false; + + continue; } + + // Otherwise we don't know what this is, not ok. + return false; + } + + return true; +} + + +/// AllGlobalLoadUsesSimpleEnoughForHeapSRA - If all users of values loaded from +/// GV are simple enough to perform HeapSRA, return true. +static bool AllGlobalLoadUsesSimpleEnoughForHeapSRA(GlobalVariable *GV, + MallocInst *MI) { + SmallPtrSet AnalyzedLoadUsingPHIs; + for (Value::use_iterator UI = GV->use_begin(), E = GV->use_end(); UI != E; + ++UI) + if (LoadInst *LI = dyn_cast(*UI)) + if (!LoadUsesSimpleEnoughForHeapSRA(LI, MI, AnalyzedLoadUsingPHIs)) + return false; return true; } @@ -1166,7 +1187,7 @@ static void RewriteHeapSROALoadUser(LoadInst *Load, Instruction *LoadUser, /// RewriteUsesOfLoadForHeapSRoA - We are performing Heap SRoA on a global. Ptr /// is a value loaded from the global. Eliminate all uses of Ptr, making them /// use FieldGlobals instead. All uses of loaded values satisfy -/// GlobalLoadUsesSimpleEnoughForHeapSRA. +/// AllGlobalLoadUsesSimpleEnoughForHeapSRA. static void RewriteUsesOfLoadForHeapSRoA(LoadInst *Load, const std::vector &FieldGlobals) { std::vector InsertedLoadsForPtr; @@ -1367,7 +1388,7 @@ static bool TryToOptimizeStoreOfMallocToGlobal(GlobalVariable *GV, // This the structure has an unreasonable number of fields, leave it // alone. if (AllocSTy->getNumElements() <= 16 && AllocSTy->getNumElements() != 0 && - GlobalLoadUsesSimpleEnoughForHeapSRA(GV, MI)) { + AllGlobalLoadUsesSimpleEnoughForHeapSRA(GV, MI)) { // If this is a fixed size array, transform the Malloc to be an alloc of // structs. malloc [100 x struct],1 -> malloc struct, 100 diff --git a/test/Transforms/GlobalOpt/2008-12-16-HeapSRACrash-2.ll b/test/Transforms/GlobalOpt/2008-12-16-HeapSRACrash-2.ll new file mode 100644 index 00000000000..cdc27714c79 --- /dev/null +++ b/test/Transforms/GlobalOpt/2008-12-16-HeapSRACrash-2.ll @@ -0,0 +1,24 @@ +; RUN: llvm-as < %s | opt -globalopt | llvm-dis +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128" +target triple = "i386-apple-darwin7" + %struct.foo = type { i32, i32 } +@X = internal global %struct.foo* null ; <%struct.foo**> [#uses=2] + +define void @bar(i32 %Size) nounwind noinline { +entry: + %tmp = malloc [1000000 x %struct.foo] ; <[1000000 x %struct.foo]*> [#uses=1] + %.sub = getelementptr [1000000 x %struct.foo]* %tmp, i32 0, i32 0 ; <%struct.foo*> [#uses=1] + store %struct.foo* %.sub, %struct.foo** @X, align 4 + ret void +} + +define i32 @baz() nounwind readonly noinline { +bb1.thread: + %tmpLD1 = load %struct.foo** @X, align 4 ; <%struct.foo*> [#uses=2] + br label %bb1 + +bb1: ; preds = %bb1, %bb1.thread + %tmp = phi %struct.foo* [ %tmpLD1, %bb1.thread ], [ %tmpLD1, %bb1 ] ; <%struct.foo*> [#uses=1] + %0 = getelementptr %struct.foo* %tmp, i32 1 ; <%struct.foo*> [#uses=0] + br label %bb1 +}