[PATCH] D80399: [Local] Prevent `invertCondition` from creating a redundant instruction

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:3067-3068
-    if (isa<PHINode>(Inst)) {
-      // FIXME: This fails if the inversion is to be used in a
-      // subsequent PHINode in the same basic block.
-      Inverted->insertBefore(&*Parent->getFirstInsertionPt());
----------------
sameerds wrote:
> foad wrote:
> > sameerds wrote:
> > > I don't think the current change fixes this issue. As far as I can see, it can't even be fixed inside this function since the intended use is not visible here. It might help to move the comment to the beginning of the function so that it is more obvious to users.
> > I think the FIXME is completely misguided. PHIs are defined to happen simultaneously so there is no "subsequent PHINode".
> I don't have a concrete example, so I'll put this as a question: Even if all PHI's "happen" simultaneously, they do "occur" in a sequence. If there are two phi's one after the other, then does the first phi "dominate" the next phi? If yes, it can potentially be an argument along some control flow edge. Then something complicated like the structurizer may decide to invert the value of the first phi and the insertion will fail. Is that a legal scenario?
> If there are two phi's one after the other, then does the first phi "dominate" the next phi?

No.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80399