[PATCH] D38944: [GVN] Handle removal of first implicit CF instruction correctly

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev abandoned this revision.
mkazantsev marked 2 inline comments as done.
mkazantsev added a comment.

Given that https://reviews.llvm.org/D37460 and this one should only go together, it makes more sense to merge them into one. I am abandoning this one; the fixed changes from here will be merged into https://reviews.llvm.org/D37460. Abandoning this one.



================
Comment at: lib/Transforms/Scalar/GVN.cpp:1202
       if (MD) MD->removeInstruction(I);
-      I->eraseFromParent();
+      assert(FirstImplicitControlFlowInsts.lookup(I->getParent()) != I &&
+             "Newly created instruction has implicit control flow?");
----------------
reames wrote:
> This assertion doesn't look correct.  Your message says the instruction doesn't have implicit control flow, the actual assert says it's not the first in the basic block.  One or the other needs fixed.
The intention here is to make sure that we don't need to take actions updating this map here. Actually this assertion doesn't bring in anything new because all map-related stuff is handled within `markInstructionForDeletion`.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1204
+             "Newly created instruction has implicit control flow?");
+      markInstructionForDeletion(I);
     }
----------------
reames wrote:
> This looks highly suspicious.  Why remove from MD, but not erase?  I think you want to either a) remove the MD->removeInstruction (because it's handled on actual deletion), or b) leave a good comment here.
> 
> Also, if replacing eraseInstruction with markForDeletion really is NFC, it should be a standalone patch which this is rebased on.
Thanks for pointing out. Submitted as separate change: https://reviews.llvm.org/D39369


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2104
+    bool InvalidateImplicitCF = false;
+    for (auto *I : InstrsToErase) {
+      assert(I->getParent() == BB && "Erasing instruction from wrong block?");
----------------
reames wrote:
> The switch to a range iteration, and addition of the assertion is a separable change.  Please commit that as a change of it's own and rebase the patch to reduce the diff for review.
Commited as separate NFC: https://reviews.llvm.org/rL316748


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2109
+      DEBUG(verifyRemoved(I));
+      if (!InvalidateImplicitCF &&
+          FirstImplicitControlFlowInsts.lookup(I->getParent()) == I)
----------------
reames wrote:
> dberlin wrote:
> > reames wrote:
> > > This can be written as:
> > > InvalidateImplicitCF |= new_condition;
> > ```|= wouldn't short circuit however, since it's bitwise and not logical
> > So it would have to be InvalidateImplicitCF = x || y
> > 
> If we're actually worried about reducing the evaluation time of the RHS - not sure we are - we can manually LICM the expression "FirstICFI.lookup(I->getParent())" since this is all the same BB.
> 
> I'm not sure the minor time savings of the short circuit is worth the code complexity honestly.  I'd also expect a decent compiler to recognize that this is actually boolean.  If we don't get that case, dang we really should.
Reworked this piece to avoid both redundan execution and dangling pointers.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2313
   DEBUG(verifyRemoved(CurInst));
+  bool InvalidateImplicitCF =
+      FirstImplicitControlFlowInsts.lookup(CurInst->getParent()) == CurInst;
----------------
reames wrote:
> Hm, optional larger code organization suggestion.  Could we introduce two operations: one which invalidates and one which rebuilds?  This would make the invariants of the FillICFI function more clear and would make this code a bit more readable.
> 
> If we could arrange to have the ICFI lazily rebuild on access to the block, that might be an overall cleaner scheme.
> 
> Again, both parts are optional.
I will consider ractoring out ICF-related stuff into a separate structure in follow-up patches.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2408
+    // Make sure that the instruction is not marked for further removal.
+    assert(std::find(InstrsToErase.begin(), InstrsToErase.end(), I) ==
+           InstrsToErase.end() && "Filling before removed all marked insns?");
----------------
reames wrote:
> This assert doesn't look like it belongs in the lamda.  Lift out to containing function.
I wasn't certain that `InstrsToErase` always contains instructions from one block at this point, but it seems it does. Hoisted the assert out.


https://reviews.llvm.org/D38944