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

[Connor Abbott via Phabricator via llvm-commits]

cwabbott added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:2203
         if (OldPhiNodes.insert(PNode))
           PhiWorklist.push_back(PNode);
         continue;
----------------
lebedev.ri wrote:
> 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.
I'd agree with @nikic here that this sounds like a premature optimization. It would split up the two parts which must be in sync into three, and break the symmetry between them, making it even harder to verify that the already-complex transform is correct, all for a theoretical performance benefit. I can do it if you require it, but otherwise I'd rather not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71209