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

[Ehud Katz via Phabricator via llvm-commits]

ekatz marked an inline comment as done.
ekatz 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());
----------------
foad wrote:
> 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.
A PHI function ("Node" for our purposes) receives a block as an argument, which then redirects to a value. This is done at the beginning of a block, as if "before entering it". It is actually equivalent to MLIR "block arguments" (sending arguments to a basic block, the same way as sent to a function).
Arguments are precalculated in the caller's block, and sent as they are to the new block. They cannot have dependency on each other.
A PHI node cannot be dependent on another PHI node that doesn't come from the exit of a basic-block to the entry of the node's parent basic-block.
Hope I made it clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80399