[PATCH] D68006: DSE miscompile when store is clobbered across loop iterations

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:625
 
   // Start checking the store-block.
+  WorkList.emplace_back(SecondBB, /*VisitedThroughBackedge=*/false);
----------------
Isn't store-block confusing?


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:630
 
   // Check all blocks going backward until we reach the load-block.
   while (!WorkList.empty()) {
----------------
Load-block?


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:636
 
     // Ignore instructions before LI if this is the FirstBB.
     BasicBlock::iterator BI = (B == FirstBB ? FirstBBI : B->begin());
----------------
Is LI an old name for FirstI or FirstBBI? There is several more refs to LI and SI in the code below. Makes it a little bit hard to follow what is going on here, since the code/comments are rotten already in the baseline. Would not mind if that was cleaned up in a separate commit.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:641
     if (isFirstBlock) {
       // Ignore instructions after SI if this is the first visit of SecondBB.
       assert(B == SecondBB && "first block is not the store block");
----------------
Is SI an old name for SecondI? Or SecondBBI?


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:666
       for (auto PredI = pred_begin(B), PE = pred_end(B); PredI != PE; ++PredI) {
-        if (!Visited.insert(*PredI).second)
+        auto *Pred = *PredI;
+        if (!Visited.insert(Pred).second)
----------------
nit: I don't think that we need this local variable. Just use *PredI instead. If you keep it, then be explicit about the type instead of using auto. (As currently written you have similarly named variables that more or less can denote the same thing, but without really explaining the difference by having any type information. Just confusing IMHO.)


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:670
+        bool IsBackedge = Backedges.count(std::make_pair(B, Pred));
+        WorkList.emplace_back(Pred, VisitedThroughBackedge | IsBackedge);
       }
----------------
If we have a cfg such as

A: (firstBB)
B: preds A, D
C: preds B (secondBB)
D: preds C

then I think we only visit block B once with this solution. And with VisitedThroughBackedge being false. It is only the SecondBB that can be visited twice afaict. Isn't that a problem?




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

https://reviews.llvm.org/D68006