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

[David Majnemer via llvm-commits]

majnemer added a comment.

I think this can be N**2 if there is a big tree of phi nodes with bitcasts at different levels of the tree.  It might make sense to limit the total amount of work we are willing to do this combine.

How many items of work does the worklist typically process before firing?


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1803-1804
@@ +1802,4 @@
+
+  SmallVector<PHINode*, 4> Worklist;
+  SmallSetVector<PHINode*, 4> OldPhiNodes;
+
----------------
Pointers lean right.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1821
@@ +1820,3 @@
+          continue;
+        // If a LoadInst has more than one use, change the type of loaded value
+        // may create another bitcast.
----------------
change -> changing

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1826
@@ +1825,3 @@
+
+      PHINode *PNode = dyn_cast<PHINode>(IncValue);
+      if (PNode) {
----------------
Please use `auto *`

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1833
@@ +1832,3 @@
+
+      BitCastInst *BCI = dyn_cast<BitCastInst>(IncValue);
+      // We can't handle other instructions.
----------------
Please use `auto *`

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1847
@@ +1846,3 @@
+  // For each old PHI node, create a corresponding new PHI node with a type A.
+  SmallDenseMap<PHINode*, PHINode*> NewPNodes;
+  for (auto OldPN : OldPhiNodes) {
----------------
Pointers lean right.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1848
@@ +1847,3 @@
+  SmallDenseMap<PHINode*, PHINode*> NewPNodes;
+  for (auto OldPN : OldPhiNodes) {
+    Builder->SetInsertPoint(OldPN);
----------------
Here, I'd use `PHINode *` or at least `auto *`.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1855
@@ +1854,3 @@
+  // Fill in the operands of new PHI nodes.
+  ConstantFolder CFolder;
+  for (auto OldPN : OldPhiNodes) {
----------------
Why not just use the IRBuilder? It should appropriately forward to the constant folder.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1856
@@ +1855,3 @@
+  ConstantFolder CFolder;
+  for (auto OldPN : OldPhiNodes) {
+    PHINode *NewPN = NewPNodes[OldPN];
----------------
Here, I'd use `PHINode *` or at least `auto *`.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1861
@@ +1860,3 @@
+      Value *NewV = nullptr;
+      if (Constant *C = dyn_cast<Constant>(V)) {
+        NewV = CFolder.CreateBitCast(C, DestTy);
----------------
Please use `auto *`.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1863
@@ +1862,3 @@
+        NewV = CFolder.CreateBitCast(C, DestTy);
+      } else if (LoadInst *LI = dyn_cast<LoadInst>(V)) {
+        Builder->SetInsertPoint(OldPN->getIncomingBlock(j)->getTerminator());
----------------
Please use `auto *`.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1866
@@ +1865,3 @@
+        NewV = Builder->CreateBitCast(LI, DestTy);
+      } else if (BitCastInst *BCI = dyn_cast<BitCastInst>(V)) {
+        NewV = BCI->getOperand(0);
----------------
Please use `auto *`.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1868
@@ +1867,3 @@
+        NewV = BCI->getOperand(0);
+      } else if (PHINode *PrevPN = dyn_cast<PHINode>(V)) {
+        NewV = NewPNodes[PrevPN];
----------------
Ditto.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1878
@@ +1877,3 @@
+  for (User *U : PN->users()) {
+    StoreInst *SI = dyn_cast<StoreInst>(U);
+    if (SI && SI->getOperand(0) == PN) {
----------------
Ditto.


http://reviews.llvm.org/D14596