[PATCH] [SROA] Simplify PHI nodes before promoting the alloca

[Jingyue Wu]

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3101
@@ +3100,3 @@
+/// This function performs a standard depth-first search along the use-def
+/// chains of the PHI nodes. If the searching reaches a non-PHI other than
+/// Source, it stops and returns true.
----------------
Eli Bendersky wrote:
> by "of the PHI nodes" do you mean "PN"?
Changed the comment to "incoming values of PN".

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3102
@@ +3101,3 @@
+/// chains of the PHI nodes. If the searching reaches a non-PHI other than
+/// Source, it stops and returns true.
+static bool
----------------
Chandler Carruth wrote:
> Eli Bendersky wrote:
> > Please document all the arguments of this function
> Wo don't actually insist on that usually. I'd rather the arguments have descriptive names than comments personally.
done

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3106
@@ +3105,3 @@
+                           SmallPtrSetImpl<PHINode *> &OriginatesFromOther,
+                           Value *Source) {
+  if (Visited.count(PN))
----------------
Mark Heffernan wrote:
> Couple bike-shed suggestions:
> 
> Maybe name the function PHINodeEquivalentToSource?
> 
> You could also replace both ptr sets with a map<PHINode *, bool> where membership indicates it's been visited and the value indicates equivalence.
Merged these data structures to one DenseMap, and renamed the function to PHINodeEquivalentToNewAlloca.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3140
@@ +3139,3 @@
+  for (auto PN : PHIUsers) {
+    if (!PHINodeOriginatesFromOther(PN, Visited, OriginatesFromOther, NewAI))
+      ToDelete.insert(PN);
----------------
Eli Bendersky wrote:
> Does the logic have to be inverted? If you only use this method in a "!" clause, can't it check positively - i.e. origination from only NewAI? This can make reasoning about it simpler.
done

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3141
@@ +3140,3 @@
+    if (!PHINodeOriginatesFromOther(PN, Visited, OriginatesFromOther, NewAI))
+      ToDelete.insert(PN);
+  }
----------------
Mark Heffernan wrote:
> If you use a map<PHINode *, bool> for equivalence as mentioned above, you can cleanly do away with ToDelete.  Just iterate through the map and replace the nodes with a true value.  I, think, necessarily those will all be in PHIUsers.
Agreed. Done. 

================
Comment at: lib/Transforms/Scalar/SROA.cpp:3143
@@ +3142,3 @@
+  }
+  for (auto PN: ToDelete) {
+    PHIUsers.erase(PN);
----------------
Eli Bendersky wrote:
> whitespace around ":"
Ack'ed.

http://reviews.llvm.org/D4659