[PATCH] D14596: [SROA] Choose more profitable type in findCommonType

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1846
@@ +1845,3 @@
+  ConstantFolder CFolder;
+  for (auto OldPN : Visited) {
+    PHINode *NewPN = NewPNodes[OldPN];
----------------
chandlerc wrote:
> davidxl wrote:
> > the two loops can be fused together.
> This is hard, because in the current structure you might need a new phi node to use as the input to another phi node. And this can't be solved because phi nodes are inherently cyclic.
> 
> I think you will have to build all the nodes first, and then fill in their operands.
> 
> However, the current code has a problem where the iteration order isn't stable. I would suggest using a SetVector to build the list of phi nodes, and I would give it a better name than "visited" to make it clear that we're going to use it here to rewrite the nodes. Maybe just "PhiNodes" or "NodeSet" to make it clear that we're going to keep using this set past the worklist walk.
> 
> There is other code in instcombine that deals with phi node cycles, and it might be useful to check how they handle these patterns and name things (I don't know personally...)
Right -- there is indeed a loop carried dependency I missed at the first look.


http://reviews.llvm.org/D14596