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

More trace optimisations #1552

Merged
merged 9 commits into from
Jan 20, 2025
Merged

More trace optimisations #1552

merged 9 commits into from
Jan 20, 2025

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Jan 16, 2025

This PR implements a number of new trace optimisations (most of which I realised are necessary by tracing some Lua programs with yklua), and tidies up some of the trace optimiser along the way. Each commit should be self-contained.

This is clearly a copy and paste error -- though thankfully a harmless
one!
These should thus be `panic`s -- we should never have anything to
implement here.
I noticed (because I made a mistake!) that we weren't testing the
canonicalisation cases properly. This commit looks like it changes much
more than it really does: basically we add a comment to the main
optimiser code which causes lots of indenting and churn without any
meaningful changes. The meat of the commit is the test cases at the end.
@@ -415,6 +415,16 @@ impl Module {
self.insts[usize::from(iidx)] = inst;
}

/// Replace the instruction it `iidx` with an instruction that will generate `op`. In other
Copy link
Contributor

Choose a reason for hiding this comment

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

s/it/in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 762413f.

@ptersilie
Copy link
Contributor

Please squash.

@ltratt
Copy link
Contributor Author

ltratt commented Jan 17, 2025

Squashed.

@ptersilie ptersilie added this pull request to the merge queue Jan 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2025
@ltratt
Copy link
Contributor Author

ltratt commented Jan 17, 2025

Hmm, with tryci only Bounce (surprise, surprise) fails. Maybe try again?

@ptersilie ptersilie added this pull request to the merge queue Jan 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2025
@ltratt
Copy link
Contributor Author

ltratt commented Jan 17, 2025

Oof, it seems we are probably hitting a codegen problem with xor and i1. I think this reflects a deep flaw in our IR where we use byte size instead of bit size. I can fix this, but it won't be quick. My suggestion is that I remove the Xor commit (which will cause some conflicts with the next commit), force push, and then I can fix the deeper problem.

@ltratt
Copy link
Contributor Author

ltratt commented Jan 17, 2025

OK, so this is actually because we (well... the perpendicular pronoun would be more accurate) didn't consider signed/zero extension in constants in the codegen. I have a partial fix but I need to audit the whole module for other instances. That's a tomorrow job.

@ltratt
Copy link
Contributor Author

ltratt commented Jan 18, 2025

#1553 fixes a problem that happens to manifest in this PR (but isn't caused by this PR) with havlak. When #1553 merges, we should be able to retry this PR, but Bounce may still cause us problems.

@ltratt
Copy link
Contributor Author

ltratt commented Jan 20, 2025

@ptersilie I think this should now merge.

@ptersilie ptersilie added this pull request to the merge queue Jan 20, 2025
Merged via the queue into ykjit:master with commit e6a10cf Jan 20, 2025
2 checks passed
@ltratt ltratt deleted the more_trace_opts branch January 20, 2025 11:12
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