This is an archive of the discontinued LLVM Phabricator instance.

[Local] Prevent `invertCondition` from creating a redundant instruction
ClosedPublic

Authored by ekatz on May 21 2020, 12:23 PM.

Details

Summary

Prevent invertCondition from creating the inversion instruction, in case the given value is an argument which has already been inverted.
Note that this approach has already been taken in case the given value is an instruction (and not an argument).

Diff Detail

Event Timeline

ekatz created this revision.May 21 2020, 12:23 PM

Of course, this requires also fixing the tests affected by this change; but I wanted to present the change first, and if approved, I'll work on fixing the tests.

I support this enhancement. Thanks for doing it!

llvm/lib/Transforms/Utils/Local.cpp
3067–3068

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.

3072

This name change affects a lot of FileCheck directives. I was there once, and backed off very very slowly. :D

foad added a comment.May 22 2020, 3:23 AM

Looks great to me!

llvm/lib/Transforms/Utils/Local.cpp
3067–3068

I think the FIXME is completely misguided. PHIs are defined to happen simultaneously so there is no "subsequent PHINode".

sameerds added inline comments.May 22 2020, 4:22 AM
llvm/lib/Transforms/Utils/Local.cpp
3067–3068

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?

foad added inline comments.May 22 2020, 4:52 AM
llvm/lib/Transforms/Utils/Local.cpp
3067–3068

If there are two phi's one after the other, then does the first phi "dominate" the next phi?

No.

ekatz marked an inline comment as done.May 24 2020, 1:08 AM
ekatz added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
3067–3068

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.

ekatz updated this revision to Diff 266271.May 26 2020, 10:45 AM

Updated tests to fit the change.

sameerds accepted this revision.May 27 2020, 1:11 AM

LGTM!

This revision is now accepted and ready to land.May 27 2020, 1:11 AM
foad added a comment.May 27 2020, 1:27 AM

It looks like all those test diffs are caused by naming values. Can you add (perhaps even precommit) a test where your change makes a real improvement?

ekatz added a comment.May 27 2020, 2:51 AM

It looks like all those test diffs are caused by naming values. Can you add (perhaps even precommit) a test where your change makes a real improvement?

I already have another change that have tests affected by this one. Please take a look at https://reviews.llvm.org/D79037.

This revision was automatically updated to reflect the committed changes.