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

@@ -1012,56 +1012,77 @@ static void ReplaceUsesOfMallocWithGlobal(Instruction *Alloc,
   }
 }
 
-/// GlobalLoadUsesSimpleEnoughForHeapSRA - If all users of values loaded from
+/// LoadUsesSimpleEnoughForHeapSRA - Verify that all uses of V (a load, or a phi
-/// GV are simple enough to perform HeapSRA, return true.
+/// of a load) are simple enough to perform heap SRA on.  This permits GEP's
-static bool GlobalLoadUsesSimpleEnoughForHeapSRA(GlobalVariable *GV,
+/// that index through the array and struct field, icmps of null, and PHIs.
-                                                 MallocInst *MI) {
+static bool LoadUsesSimpleEnoughForHeapSRA(Value *V, MallocInst *MI,
-  for (Value::use_iterator UI = GV->use_begin(), E = GV->use_end(); UI != E; 
+                             SmallPtrSet<PHINode*, 32> &AnalyzedLoadUsingPHIs) {
-       ++UI)
+  // We permit two users of the load: setcc comparing against the null
-    if (LoadInst *LI = dyn_cast<LoadInst>(*UI)) {
+  // pointer, and a getelementptr of a specific form.
-      // We permit two users of the load: setcc comparing against the null
+  for (Value::use_iterator UI = V->use_begin(), E = V->use_end(); UI != E;++UI){
-      // pointer, and a getelementptr of a specific form.
+    Instruction *User = cast<Instruction>(*UI);
-      for (Value::use_iterator UI = LI->use_begin(), E = LI->use_end();
+    
-           UI != E; ++UI) {
+    // Comparison against null is ok.
-        // Comparison against null is ok.
+    if (ICmpInst *ICI = dyn_cast<ICmpInst>(User)) {
-        if (ICmpInst *ICI = dyn_cast<ICmpInst>(*UI)) {
+      if (!isa<ConstantPointerNull>(ICI->getOperand(1)))
-          if (!isa<ConstantPointerNull>(ICI->getOperand(1)))
+        return false;
-            return false;
+      continue;
-          continue;
+    }
-        }
+    
+    // getelementptr is also ok, but only a simple form.
+    if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(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<PHINode>(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 the phi'd value is from the malloc that initializes the value,
-        if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(*UI)) {
+        // we can xform it.
-          // Must index into the array and into the struct.
+        if (IV == MI) continue;
-          if (GEPI->getNumOperands() < 3)
-            return false;
-          
-          // Otherwise the GEP is ok.
-          continue;
-        }
         
-        if (PHINode *PN = dyn_cast<PHINode>(*UI)) {
+        // Otherwise, we don't know what it is.
-          // 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.
         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<PHINode*, 32> AnalyzedLoadUsingPHIs;
+  for (Value::use_iterator UI = GV->use_begin(), E = GV->use_end(); UI != E; 
+       ++UI)
+    if (LoadInst *LI = dyn_cast<LoadInst>(*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<GlobalVariable*> &FieldGlobals) {
   std::vector<Value *> 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

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
+}