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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2109
+      DEBUG(verifyRemoved(I));
+      if (!InvalidateImplicitCF &&
+          FirstImplicitControlFlowInsts.lookup(I->getParent()) == I)
----------------
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.


https://reviews.llvm.org/D38944