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

C++: Add Parameter nodes for functions without bodies #16246

Merged

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Apr 17, 2024

This PR fixes an annoyance with ParameterNodes in C/C++ dataflow: Parameters of functions without a definition (i.e., when we only have a set of declarations) don't generate ParameterNodes.

This is because we define ParameterNodes using the InitializeParameter instruction in the IR, and without a function body there's no instructions associated with the function.

This PR fixes this by generating new dataflow nodes (from Parameters) and marking those as ParameterNodes.

Ideally, only 98a3f2d and 06f52c2 would've been necessary. However, the current setup for marking parameter nodes wasn't very easy to extend to something that wasn't defined in terms of Instructions so it took some refactoring (see 111ad8b) to allow for this. That commit should be totally behavior preserving.

Commit-by-commit review recommended.

cc @bdrodes

@github-actions github-actions bot added the C++ label Apr 17, 2024
@MathiasVP MathiasVP force-pushed the parameter-nodes-for-functions-without-bodies branch from b6a9e7d to b43aae1 Compare April 18, 2024 11:03
@MathiasVP MathiasVP marked this pull request as ready for review April 18, 2024 11:54
@MathiasVP MathiasVP requested a review from a team as a code owner April 18, 2024 11:54
@MathiasVP MathiasVP force-pushed the parameter-nodes-for-functions-without-bodies branch from e3bf902 to bcda4a1 Compare April 18, 2024 12:03
@jketema
Copy link
Contributor

jketema commented Apr 19, 2024

I'm missing something - I think. How do we know that the bodiless parameter nodes are actually only for functions without bodies? I don't see an explicit condition for this anywhere.

@MathiasVP
Copy link
Contributor Author

I'm missing something - I think. How do we know that the bodiless parameter nodes are actually only for functions without bodies? I don't see an explicit condition for this anywhere.

That's the not any(InitializeParameterInstruction init).getParameter() = p part that's in the IPA branch for the new dataflow nodes. At least, that's what I intended when I wrote it 😂

@jketema
Copy link
Contributor

jketema commented Apr 19, 2024

I'm missing something - I think. How do we know that the bodiless parameter nodes are actually only for functions without bodies? I don't see an explicit condition for this anywhere.

That's the not any(InitializeParameterInstruction init).getParameter() = p part that's in the IPA branch for the new dataflow nodes. At least, that's what I intended when I wrote it 😂

Ah, my brain filtered that out completely because of the horrendous formatting of the diff on my small screen 🤦

@MathiasVP MathiasVP merged commit bcedf68 into github:main Apr 19, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants