[PATCH] D38944: [GVN] Handle removal of first implicit CF instruction correctly
Philip Reames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 26 14:50:58 PDT 2017
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
More information about the llvm-commits
mailing list