[PATCH] D68032: Handle successor's PHI node correctly when flattening CFG merges two if-regions

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/FlattenCFG.cpp:457
+  // Handle PHI node to replace its predecessors to FirstEntryBlock.
+  for (unsigned int i = 0; i < PBI->getNumSuccessors(); ++i) {
+    BasicBlock *Successor = PBI->getSuccessor(i);
----------------
nit: `for (unsigned int i = 0; i < PBI->getNumSuccessors(); ++i)`
->
`for (BasicBlock *Succ : successors(PBI))`


================
Comment at: llvm/lib/Transforms/Utils/FlattenCFG.cpp:459
+    BasicBlock *Successor = PBI->getSuccessor(i);
+    PHINode *SomePHI = dyn_cast<PHINode>(Successor->begin());
+    if (SomePHI) {
----------------
I think this should iterate over all the phi nodes in the successor basic block?
You can get that with: `BB->phis()`


================
Comment at: llvm/lib/Transforms/Utils/FlattenCFG.cpp:460
+    PHINode *SomePHI = dyn_cast<PHINode>(Successor->begin());
+    if (SomePHI) {
+      for (unsigned int j = 0; j < SomePHI->getNumIncomingValues(); ++j) {
----------------
LLVM prefers quick exists; I think it would be better to use:
```
if (!SomePhi)
  continue;
```


================
Comment at: llvm/lib/Transforms/Utils/FlattenCFG.cpp:461
+    if (SomePHI) {
+      for (unsigned int j = 0; j < SomePHI->getNumIncomingValues(); ++j) {
+        if (SomePHI->getIncomingBlock(j) == SecondEntryBlock)
----------------
nit: `for (unsigned int j = 0; j < SomePHI->getNumIncomingValues(); ++j)`
-->
`for (unsigned i = 0, e = SomePHI->getNumIncomingValues(); i != e; ++i)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68032/new/

https://reviews.llvm.org/D68032