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

[Guozhi Wei via llvm-commits]

Carrot added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1846
@@ +1845,3 @@
+  ConstantFolder CFolder;
+  for (auto OldPN : Visited) {
+    PHINode *NewPN = NewPNodes[OldPN];
----------------
davidxl wrote:
> 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.
I found the code in function InstCombiner::SliceUpIllegalIntegerPHI, it has similar code structure. It uses a SmallPtrSet variable named PHIsInspected, but it is not used for other purposes.

I changed my var to type SmallSetVector, and named as "OldPhiNodes".

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1863-1864
@@ +1862,4 @@
+
+  // If there is a store with type B, change it to type A.
+  SmallVector<StoreInst *, 4> ToBeDeleted;
+  for (User *U : PN->users()) {
----------------
chandlerc wrote:
> Hmm, if you handle stores in addition to bitcasts for phi node users, you should handle loads in addition to bitcasts for phi node operands.
> 
> However, you shouldn't need to rewrite the loads and stores. Instead, you should be able to introduce a bitcast for the loads and stores, and remove the other bitcasts. Then instcombine should trigger the existing code to fold the loads and stores through the casts.
> 
> The only thing to watch out for here are introducing cycles where we switch bitcasts from one place to another on phi nodes. I'm not sure how best to do that. Maybe David Majnemer has some ideas here?
For the load case, I handle the load instruction that has only one use. Otherwise it may create another bitcast for other uses, it may not be good for performance.


http://reviews.llvm.org/D14596