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

Pass to reduce added flags from RemoveReturn #4878

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

ChrisDodd
Copy link
Contributor

This adds a pass running before RemoveReturns that rewrites code with a conditional jump (a break/continue/exit/return in a if branch) where there's code after the if statement that will only run if the if condition is false. It moves that following code into the else (ifFalse) part of the IfStatement

The point of this is avoid needing to introduce a flag variable in the later RemoveReturns or RemoveExits. It can also help with loops.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Aug 21, 2024
@fruffy fruffy added the run-validation Use this tag to trigger a Validation CI run. label Aug 22, 2024
@jafingerhut
Copy link
Contributor

@fruffy Does something like gauntlet run automatically on all PRs? Or is there a way to manually trigger it on a particular PR by adding a tag? This PR seems likes a good one to have that extra checking, if it is easy to enable.

@fruffy
Copy link
Collaborator

fruffy commented Aug 23, 2024

@fruffy Does something like gauntlet run automatically on all PRs? Or is there a way to manually trigger it on a particular PR by adding a tag? This PR seems likes a good one to have that extra checking, if it is easy to enable.

Yes, I added the run-validation tag which will run the tool once the pull request is updated. It will also run nightly after it has been merged.

Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

Caveats: I did not review the C++ code changes. I also did not carefully review most of the expected output P4C changes. They are simply too long to do so in a reasonable amount of time, hence my question of Fabian about adding the run-validation tag to get better automated coverage of the correctness of those changes.

Thus this is a very "weak" LGTM approval, based upon me manually checking a few of the very short expected output files, and finding no problems.

If someone is willing to review parts of this PR that I have not, I would consider my approval to be not enough on its own to merge this.

@ChrisDodd
Copy link
Contributor Author

Turns out there's a bug here (incorrectly applying changes) due to ignoring the effects of switch statements (and loops), so I'll push a revision soon.

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

I like this. I've noticed the RemoveRetun pass introduces way too much flags, but did not get to figuring out why and what we can do about this :-). I just have some questions.

frontends/p4/removeReturns.h Outdated Show resolved Hide resolved
frontends/p4/removeReturns.h Outdated Show resolved Hide resolved
frontends/p4/removeReturns.cpp Show resolved Hide resolved
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Maybe just also rename hasBranched to hasJumped or something like that.

- a pass that rewrites code after an if the branches
  (return/exit/break/continue) moving the code into the else branch of
  the if.

Signed-off-by: Chris Dodd <[email protected]>
Signed-off-by: Chris Dodd <[email protected]>
@ChrisDodd ChrisDodd added this pull request to the merge queue Aug 28, 2024
Merged via the queue into p4lang:main with commit b3d59d9 Aug 28, 2024
18 checks passed
@ChrisDodd ChrisDodd deleted the cdodd-elserewrite branch August 28, 2024 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) run-validation Use this tag to trigger a Validation CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants