[PATCH] D11143: [RFC] Cross Block DSE

[hfinkel at anl.gov]

hfinkel added inline comments.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:61
@@ +60,3 @@
+        BackEdges;
+    std::map<std::pair<const BasicBlock *, const BasicBlock *>, bool>
+        BackEdgesMap;
----------------
Use a DenseSet (I assume you're not depending on any ordering from std::map because it is storing pointers).

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:71
@@ -57,1 +70,3 @@
 
+    // Return all stores in a give BasicBlock.
+    SmallVector<StoreInst *, 8> getStores(BasicBlock *BB) {
----------------
give -> given

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:74
@@ +73,3 @@
+      SmallVector<StoreInst *, 8> VecStores;
+      for (auto BI = BB->end(), BE = BB->begin(); BI != BE;) {
+        Instruction *Ins = --BI;
----------------
You can use a range-based for loop here.

  for (const auto &I : BB) {
    ...
  }

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:85
@@ +84,3 @@
+    void populateCandidateStores(Function &F) {
+      for (Function::iterator I = F.begin(), E = F.end(); I != E; ++I) {
+        DomTreeNode *DTNode = PDT->getNode(I);
----------------
Range-based for.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:946
@@ +945,3 @@
+                                     BasicBlock *SinkBlock, StoreInst *SI) {
+  std::stack<BasicBlock *> Stck;
+  std::map<BasicBlock *, bool> Visited;
----------------
Use SmallVector<BasicBlock *, 16> (or some number you feel is appropriate).


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:947
@@ +946,3 @@
+  std::stack<BasicBlock *> Stck;
+  std::map<BasicBlock *, bool> Visited;
+  Value *Pointer = SI->getPointerOperand();
----------------
Use DenseSet, or SmallPtrSet if the size will normally be small.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:954
@@ +953,3 @@
+    Instruction *I = BBI;
+    if (StoreInst *Store = dyn_cast<StoreInst>(I)) {
+      if (Store == SI)
----------------
You don't need to do a cast here, you can compare a StoreInst* to an Instruction*, and they'll be equal only if they point to the same object (one is a base class of the other).


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:991
@@ +990,3 @@
+  while (!Stck.empty()) {
+    BasicBlock *B = Stck.top();
+    Stck.pop();
----------------
When you use a SmallVector instead, you can use pop_back_val().

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1018
@@ +1017,3 @@
+        continue;
+      // A path with backedge may not be safe.Conservatively mark
+      // this store unsafe.
----------------
Add space before 'Conservatively'

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1032
@@ +1031,3 @@
+bool DSE::isStoreDead(StoreInst *SI) {
+  for (auto I = DeadStores.begin(), E = DeadStores.end(); I != E; ++I) {
+    if (*I == SI)
----------------
Maybe you should store these in a SetVector instead to make this more efficient.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1040
@@ +1039,3 @@
+/// handleNonLocalStoreDeletion - Handle non local dead store elimination.
+/// This works by finding out candidate stroes using PDT and then running DFS
+/// from candidate store block checking all paths to make sure the store is
----------------
finding out candidate stroes -> finding candidate stores

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1054
@@ +1053,3 @@
+  for (int i = DFSNumIn + 1; i < DFSNumOut; ++i) {
+    for (auto I = Candidates[i].begin(), E = Candidates[i].end(); I != E; ++I) {
+      StoreInst *CandidateSI = *I;
----------------
Range-based for?


http://reviews.llvm.org/D11143