-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SSA: Expose phi-read nodes #11198
SSA: Expose phi-read nodes #11198
Conversation
721f329
to
2bd648a
Compare
2bd648a
to
df58aa1
Compare
df58aa1
to
d8d6165
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: Remember to revert the last "Trigger CI" commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! The Swift .expected
files need to be updated. The scary C++ CI failures can be ignored, although they might be hiding some changes to .expected
that need to be accepted.
e42b202
to
67e8ec1
Compare
@MathiasVP : I have force-pushed a change that reverts the "trigger CI" commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR follows up on #10035 by exposing "phi read" definitions in the shared SSA library.
The motivation for exposing phi reads is to be able to avoid the same potential combinatorial explosion that we also avoid by including normal phi nodes in the data flow graph:
Assuming that
A,...,F
read the same underlying variable asread_phi
, one can reduce the number of data flow edges from 9 (quadratic) to 6 (linear) by including theread_phi
join-point.This PR does not actually include read-phis in the data flow graph for any languages (follow-up work, e.g. #10927), it merely exposes them.
The old API (which ignores phi-reads) is maintained, and if one wants to take phi-reads into account, the new class
DefinitionExt
should be used instead of the existingDefinition
class. Existing predicates forDefinition
now exist in both the old version and a newExt
-suffixed version forDefinitionExt
.