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

Guard our ykllvm stackmap changes with a switch. #889

Open
vext01 opened this issue Oct 26, 2023 · 2 comments
Open

Guard our ykllvm stackmap changes with a switch. #889

vext01 opened this issue Oct 26, 2023 · 2 comments
Assignees

Comments

@vext01
Copy link
Contributor

vext01 commented Oct 26, 2023

Our stackmap changes mean that a bunch of upstream LLVM tests have to be updated in order to pass.

The problem is, updating the tests is really hard: you have to insert NextLive markers in the correct positions in the operands of the STACKMAP node in the test inputs (not the outputs). The positions to insert the markers depend on contextual information in the surrounding arguments, and some stackmap nodes have hundreds of operands.

This was caused by our fixes to make stackmaps track live variables than can span more than a single location.

When I was implementing this, I eventually gave up and started skipping tests because it was too hard. Recently Lukas has had a taste of this misery when syncing ykllvm with upstream. They had added new stackmaps tests, which then needed to be updated.

What we should do is:

  • Add a flag like --yk-extended-stackmaps and only emit our "fixed" stackmaps only if this flag is passed.
  • Revert all of our changes to upstream's stackmap tests, as they will now all pass as-is.
  • Add some of our own tests for our stackmap changes.
@vext01 vext01 self-assigned this Oct 26, 2023
@ptersilie
Copy link
Contributor

What we could do is create our own copy of the Stackmaps pass with our changes and then at compile time we just switch out the original pass with our pass guarded by a command line arg. This way we don't need to add loads of guards into Stackmaps.cpp. The downside is though that we need to manually copy over any changes to Stackmaps.cpp to our YkStackmaps.cpp on each upstream merge, which we might easily forget.

@vext01
Copy link
Contributor Author

vext01 commented Oct 26, 2023

I think I prefer to have the guards inline, as:

  • a) we are more likely to get a merge conflict when upstream changes something about stackmaps that we would need to know about.
  • b) if we ever merge this upstream, we will have less work to do.
  • c) less duplication of code.

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

No branches or pull requests

2 participants