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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Having David look at this is definitely a good thing. This will be a really nice change to instcombine, but it is in the tricky space of instcombines because it combines a fully phi-node cycle at a time, and has tricky oscillation challenges.

Some comments below.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1805
@@ +1804,3 @@
+
+  // Find out all the A->B casts and PHI nodes.
+  Worklist.push_back(PN);
----------------
nit: "Find out all the" -> "Find all of the".

The comments really make this clear, but I assume part of your intent here is to handle PHI-cycles, where a PHI node is one of its own inputs (or an indirect cycle)? It took me a while to realize that this would necessitate scanning across a worklist in this way to see if you can saturate the PHI graph with all non-PHI inputs and outputs being bitcasts or constants. I think this really would help to be laid out in some detail in a comment so that the reader of the code knows what the intended algorithm here is.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1823
@@ +1822,3 @@
+      BitCastInst *BCI = dyn_cast<BitCastInst>(IncValue);
+      if (BCI) {
+        Type *TyA = BCI->getOperand(0)->getType();
----------------
Make this an early return to reduce indentation.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1846
@@ +1845,3 @@
+  ConstantFolder CFolder;
+  for (auto OldPN : Visited) {
+    PHINode *NewPN = NewPNodes[OldPN];
----------------
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...)

================
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()) {
----------------
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?

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:2002
@@ +2001,3 @@
+  // Handle the A->B->A cast, and there is an intervening PHI node.
+  if (PHINode *PN = dyn_cast<PHINode>(Src)) {
+    if (Instruction *I = optimizeBitCastFromPhi(CI, PN))
----------------
No need for the {'s


http://reviews.llvm.org/D14596