Skip to content
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

Move phi to rvsdg #784

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move phi to rvsdg #784

wants to merge 1 commit into from

Conversation

caleridas
Copy link
Collaborator

Rename phi::node to comply with the naming scheme used for other node classes. Move it to rvsdg as it is a general-purpose vehicle to represent the fixpoint of recursive (function) definitions, and there is nothing specific to llvm within it.

@caleridas caleridas requested a review from phate January 30, 2025 18:05
@caleridas
Copy link
Collaborator Author

I tried to make the naming of context variables consistent with lambda. For the actual fixpoint variables they are akin to theta (without an input as it is infinite recursion without a real "beginning"), I am not really happy about "recref" but it is at least a bit close to what it represents.

I dared rename "recursion variables" to "fixpoint variables" as I think that "recursion is on call" while phi represents the conceptual fixpoint graph reached by dissolving and infinitely "unrolling" a phi structure. I can certainly be convinced to keep the name "RecVar" or similar, so feel free to comment if you would rather insist on that.

@caleridas caleridas force-pushed the move-phi branch 2 times, most recently from 40bcfcc to fef6967 Compare January 30, 2025 21:56
jlm/rvsdg/Phi.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/Phi.hpp Outdated Show resolved Hide resolved
class PhiBuilder;

/**
* \brief Phi node represents the fixpoint of mutually recursive definitions.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: represent

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the typo -- "phi node" is singular, so "represents" sounds correct, no?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my problem is that it was not a proper sentence. Either "A phi node represents..." or "Phi nodeS represent the fixpoint...."

jlm/rvsdg/Phi.hpp Show resolved Hide resolved
jlm/rvsdg/Phi.hpp Show resolved Hide resolved
jlm/llvm/opt/cne.cpp Outdated Show resolved Hide resolved
jlm/llvm/opt/cne.cpp Outdated Show resolved Hide resolved
jlm/llvm/opt/cne.cpp Outdated Show resolved Hide resolved
jlm/hls/opt/cne.cpp Show resolved Hide resolved
jlm/hls/opt/cne.cpp Show resolved Hide resolved
Rename phi::node to comply with the naming scheme used for other node classes.
Move it to rvsdg as it is a general-purpose vehicle to represent the fixpoint
of recursive (function) definitions, and there is nothing specific to
llvm within it.
@phate
Copy link
Owner

phate commented Feb 5, 2025

@caleridas I think you missed some review comments from the first round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants