[PATCH] D71209: InstCombine: Don't rewrite phi-of-bitcast when the phi has other users

Mon Dec 9 13:03:02 PST 2019

lebedev.ri added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:2203
         if (OldPhiNodes.insert(PNode))
           PhiWorklist.push_back(PNode);
         continue;
----------------
nikic wrote:
> lebedev.ri wrote:
> > nikic wrote:
> > > lebedev.ri wrote:
> > > > I think we should be doing those checks here.
> > > The `OldPhiNodes.count(PHI) == 0` part of the check wouldn't work correctly at this points -- it needs all PHIs to be collected first.
> > Right. But the rest can be done here, no?
> While it could be done, I don't think there'd be much benefit. We'd have to iterate all the users a second time just to check the phi nodes. The current implementation is also nicely symmetric between the checking loop (added in this patch) and the replacement loop (preexisting), and I think it's worth keeping that symmetry.
I don't have a strong opinion *here*, but in general the earlier we error-out
the less pointless work that was in vain (read: compile-time cost) we waste.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71209