[PATCH] D22143: [SimplifyCFG] Rewrite SinkThenElseCodeToEnd

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Some comments inline (and apologies for the delay!).


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1322
@@ -1321,9 +1321,3 @@
 
-/// Given an unconditional branch that goes to BBEnd,
-/// check whether BBEnd has only two predecessors and the other predecessor
-/// ends with an unconditional branch. If it is true, sink any common code
-/// in the two predecessors to BBEnd.
-static bool SinkThenElseCodeToEnd(BranchInst *BI1) {
-  assert(BI1->isUnconditional());
-  BasicBlock *BB1 = BI1->getParent();
-  BasicBlock *BBEnd = BI1->getSuccessor(0);
+static bool areValuesTriviallySame(Value *V0, BasicBlock::const_iterator At0,
+                                   Value *V1, BasicBlock::const_iterator At1) {
----------------
I think this needs a comment describing what it does (especially the role of `At0` and `At1`).

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1337
@@ +1336,3 @@
+  if (!I0->mayReadOrWriteMemory())
+    return true;
+
----------------
Does this do the right thing for `alloca` instructions?  If it does, can you please add a test case?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1415
@@ +1414,3 @@
+          auto *U = *I->user_begin();
+          return isa<PHINode>(U) && U == *I0->user_begin();
+        }))
----------------
The `isa<PHINode>(U)` seems repeated -- why not:

```
if (I0 is not store) {
  auto *PNUse = dyn_cast<>(*I0->user_begin());
  if (!PNUse || !all_of(Insts, [](I) { return *I->user_begin == PNUse; }))
    return false;
}
```


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1420
@@ +1419,3 @@
+  NumPHIsRequired = 0;
+  for (unsigned O = 0, E = I0->getNumOperands(); O != E; ++O) {
+    if (!llvm::all_of(Insts, [&I0, O](const Instruction *I) {
----------------
Very minor, but I'd s/`O`/`OI` and s/`E`/`OE`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1423
@@ +1422,3 @@
+          return areValuesTriviallySame(I->getOperand(O), I->getIterator(),
+                                        I0->getOperand(O), I0->getIterator());
+        }))
----------------
Does this work with token types (which cannot be PHI'ed)?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1444
@@ +1443,3 @@
+  for (auto *BB : Blocks)
+    Insts.push_back(&*std::prev(BB->getTerminator()->getIterator()));
+
----------------
Can't you use `getPrevNode()` here and earlier?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1470
@@ +1469,3 @@
+                               Op->getName() + ".sink", &BBEnd->front());
+    for (auto *I : Insts)
+      PN->addIncoming(I->getOperand(O), I->getParent());
----------------
You at least need to handle token types here.


Repository:
  rL LLVM

https://reviews.llvm.org/D22143