[PATCH] D23225: [ADCE] Modify data structures to support removing control flow

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

No tests?


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:40
@@ +39,3 @@
+// This is a tempoary option until we change the interface
+// to this pass based on optimiation level.
+static cl::opt<bool> RemoveControlFlowFlag("adce-remove-control-flow",
----------------
"optimization"

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:70
@@ +69,3 @@
+  TerminatorInst *Terminator = nullptr; ///< Cache of BB->getTerminator().
+};
+
----------------
Is this struct supposed to evolve in the future?
I feel it is close to the point where it may become a proper class.
(It seems to have invariants like `Terminator == BB->getTerminator`)

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:76
@@ +75,3 @@
+  /// Per basic block information.
+  std::vector<BlockInfoType> BlockInfoVec;
+
----------------
nadav wrote:
> Are you using std::vector instead of SmallVector because you are expecting that most function will have a large number of basic blocks? 
Why do you need this?
Is it because the iteration ordering over the `BlockInfo` `DenseMap` isn't deterministic? 
Why can't you always iterate over the function? Will you mutate the ordering in the vector or remove elements?

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:146
@@ +145,3 @@
+  BlockInfoVec.resize(NumBlocks);
+  BlockInfo.grow(2 * NumBlocks);
+  auto InfoP = BlockInfoVec.begin();
----------------
Use `reserve()` instead, unless you have a good reason to use `grow()` here?

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:149
@@ +148,3 @@
+  size_t NumInsts = 0;
+  for (auto &BB : F) {
+    NumInsts += BB.size();
----------------
One line comment for this loop.

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:160
@@ +159,3 @@
+  // initialize instruction map and set pointers to block info
+  InstInfo.grow(2 * NumInsts);
+  for (auto &BBInfo : BlockInfoVec)
----------------
(Same as above)

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:195
@@ +194,3 @@
+  // Treat the entry block as always live
+  auto BB = &F.getEntryBlock();
+  auto &EntryInfo = *BlockInfo[BB];
----------------
`auto *BB` to mark explicitly that it is a pointer.

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:221
@@ +220,3 @@
+    return false;
+  }
+  return true;
----------------
No trivial braces for the two previous if

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:257
@@ -119,1 +256,3 @@
+    // TODO -- handle PhiNodes
+  } while (!Worklist.empty());
 
----------------
Right now I don't make sense of this two-levels loop structure, because the `checkControlDependences()` operates only on `BlocksWithDeadTerminators`, which is only inserted to during `initialize()`. I assume it will change in a future patch? (Or I missed something)


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:284
@@ +283,3 @@
+
+  // Mark unconditoinal branches at the end of live
+  // blocks as live since there is no work to do for them later
----------------
"unconditional"

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:287
@@ +286,3 @@
+  if (BBInfo.UnconditionalBranch)
+    if (I != BBInfo.Terminator)
+      markLive(BBInfo.Terminator);
----------------
Fuse the two if with an `&&`

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:323
@@ +322,3 @@
+  SmallVector<TerminatorInst *, 16> DeadTerminators;
+  for (auto BB : BlocksWithDeadTerminators)
+    DeadTerminators.push_back(BB->getTerminator());
----------------
`auto *BB`


https://reviews.llvm.org/D23225