[PATCH] D22558: Helper functions to verify SESE, SEME and copySEME

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:236
@@ +235,3 @@
+/// a single-entry-single-exit (SESE) region. All the traces
+/// from \p Entry to \p Exit should be dominated by \p Entry
+/// and post-dominated by \p Exit.
----------------
This names are not helped by the fact that both Entry and Exit start with "E". :)

I'd suggest spelling them out in full, like `isSingleEntrySingleExit`.

Also, we don't use `\brief` in new doxygen comments, we build with autobrief now.

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:758
@@ +757,3 @@
+  for (BasicBlock *BB : Exits) {
+    for (Instruction &Inst : *BB) {
+      PHINode *PN = dyn_cast<PHINode>(&Inst);
----------------
Won't you also have to insert PHI nodes?  If this depends on LCSSA somehow then that should be documented, and asserted if possible.

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:773
@@ +772,3 @@
+        else
+          // When no mapping is available it must be a constant.
+          PN->addIncoming(Op, CopyBB);
----------------
a constant or defined outside the SEME?

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:787
@@ +786,3 @@
+  // Step1: Clone the basic blocks and populate VMap.
+  BasicBlock *DomEntry = DT->getNode(Blocks[0])->getIDom()->getBlock();
+  assert(DomEntry && "no dominator");
----------------
Are you assuming that `Blocks[0]` is the entry into the SEME?  If so, then document that.  Also, what if `Blocks[0]` is the function entry and has no IDom?

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:788
@@ +787,3 @@
+  BasicBlock *DomEntry = DT->getNode(Blocks[0])->getIDom()->getBlock();
+  assert(DomEntry && "no dominator");
+
----------------
Not sure if `getBlock` actually ever returns null.  Perhaps the assert should be on the result of `getIDom`?

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:800
@@ +799,3 @@
+    Loop *BBParentLoop = BBLoop->getParentLoop();
+    if (BBParentLoop)
+      BBParentLoop->addBasicBlockToLoop(NewBB, *LI);
----------------
Don't you sometimes have to _create_ a new loop?  That is

```
for (;;)
  for (;;)  // SEME
```

Clone =>

```
for (;;) {
  for (;;)  // SEME.0
  for (;;)  // SEME.1 -> you need a new llvm::Loop for this
}
```

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:811
@@ +810,3 @@
+  for (BasicBlock *BB : Blocks) {
+    BasicBlock *NewBB = cast_or_null<BasicBlock>(VMap[BB]);
+    BranchInst *BI = dyn_cast<BranchInst>(NewBB->getTerminator());
----------------
When can `VMap[BB]` be null?

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:813
@@ +812,3 @@
+    BranchInst *BI = dyn_cast<BranchInst>(NewBB->getTerminator());
+    if (!BI)
+      continue;
----------------
What if `NewBB->getTerminator()` is a switch?  If you don't handle that case yet, please add an assert that would fail with a switch.

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:818
@@ +817,3 @@
+      BasicBlock *NewSucc = cast_or_null<BasicBlock>(VMap[BI->getSuccessor(I)]);
+      if (!NewSucc)
+        continue;
----------------
Canonical LLVM form would be:

```
if (auto *NewSucc = ...)
  BI->setSuccessor(...);
```


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:824
@@ +823,3 @@
+
+  // Step4: Update the DOM of coppied SEME. Except for the entry block its tree
+  // structure is the same as of original SEME so the dominators also follow the
----------------
*copied

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:826
@@ +825,3 @@
+  // structure is the same as of original SEME so the dominators also follow the
+  // same structural property. If the imm-dom of orig BB is not in SEME that
+  // means it is the entry block, in that case the new idom of the new BB must
----------------
Please spell out "orig" and "imm-dom".

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:831
@@ +830,3 @@
+  for (BasicBlock *BB : Blocks) {
+    BasicBlock *NewBB = cast_or_null<BasicBlock>(VMap[BB]);
+    assert(NewBB);
----------------
Instead of asserting `NewBB` is not null, just use `cast` instead of `cast_or_null`.

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:836
@@ +835,3 @@
+    BasicBlock *NewDom = cast_or_null<BasicBlock>(VMap[Dom]);
+    if (!NewDom) { // Dom does not belong to SEME => entry block.
+      EntryNewSEME = NewBB;
----------------
I would not do this here -- I'd handle the entry block separately, and only handle non-entry blocks here.  Then I'd change the `cast_or_null` above to `cast` to catch the (buggy) case where `Dom` is not in `VMap`.

================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:846
@@ +845,3 @@
+  // Step5: Adjust PHI nodes.
+  adjustExitingPhis(VMap, Exits);
+  return EntryNewSEME;
----------------
It feels odd to have a separate function for just this one step.  Is step 5 special in some way (e.g. maybe you're planning to re-use it)?


Repository:
  rL LLVM

https://reviews.llvm.org/D22558