Skip to content

Conversation

@ltratt
Copy link
Contributor

@ltratt ltratt commented Dec 24, 2025

This PR adds a new redundant load_store elimination pass (2bdc42b): it's simple, and more-or-less a direct port from jitc_yk. The main reason I've done this now is to help me understand what's needed from the optimiser API: indeed, a change was necessary (675444e).

@Fidget-Spinner My guess is that the known_bits pass might need the extended inst_committed API too. And perhaps other bits I haven't considered yet.

@ltratt
Copy link
Contributor Author

ltratt commented Dec 24, 2025

Oops, I should have added the tests in 410fa9e in the original PR. Will need squashing.

@Fidget-Spinner
Copy link
Contributor

Do you have any benchmark results? The refactoring looks fine to me.

@ltratt
Copy link
Contributor Author

ltratt commented Dec 24, 2025

I do:

 fannkuchredux/yklua/10     13979 ± 103   13005 ± 143   0.93  6.97% faster
 nbody/yklua/250000          4694 ±  93    4458 ±  65   0.95  5.03% faster
 Heightmap/yklua/2000        8338 ±  76    7983 ±  12   0.96  4.26% faster
 sieve/yklua/3000            5066 ±  47    4860 ±  28   0.96  4.06% faster
 spectralnorm/yklua/1000    12564 ±  31   12340 ±   2   0.98  1.79% faster
 cd/yklua/250               33563 ± 140   33176 ± 161   0.99  1.15% faster
 mandelbrot/yklua/500        2266 ±   4    2292 ±   3   1.01  1.16% slower

Everything else is indistinguistable (apart from list, whose results bounce around too much to be useful). The mandelbrot result is odd but replicates at scale: there's nothing obvious causing it that I've yet understood. It may be as simple as slightly different code layout or similar...

@Fidget-Spinner
Copy link
Contributor

Ok I've taken a look at this twice now. Let's merge this.

@Fidget-Spinner
Copy link
Contributor

Please squash.

For load/store elimination, we need to know the `iidx` of the to-be
committed instruction, and access to the optimiser for `equiv_iidx`ing
and so on. This commit thus makes `inst_commited`:

```rust
fn inst_committed(&mut self, ci: &CommitInstOpt, iidx: InstIdx, inst: &Inst);
```

where `CommitInstOpt` is vaguely similar to `PassOpt` _except_
`CommitInstOpt` is `&` (not `&mut`) and therefore doesn't allow types /
instructions to be pushed. I would like to pretend that I anticipated
this API would naturally fall out of the hat from cf51a9e but I did
not. Happily, it does fall out of the hat!
@ltratt
Copy link
Contributor Author

ltratt commented Dec 24, 2025

Squashed.

@Fidget-Spinner Fidget-Spinner added this pull request to the merge queue Dec 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 24, 2025
@Fidget-Spinner
Copy link
Contributor

@ltratt the new tests are failing, happy holidays!

I'll move my branch to use the new API after this is merged.

@ltratt
Copy link
Contributor Author

ltratt commented Dec 24, 2025

Oops, fixed the doc string in the force push.

@Fidget-Spinner Fidget-Spinner added this pull request to the merge queue Dec 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 24, 2025
@Fidget-Spinner
Copy link
Contributor

lang tests are failing

@ltratt
Copy link
Contributor Author

ltratt commented Dec 25, 2025

Ah, I'm being bitten by the lack of "proper" jit-pre-opt. I'll need to chew on the best solution to this.

@ltratt
Copy link
Contributor Author

ltratt commented Dec 27, 2025

After playing for a little bit, I don't see a nice way for j2 to do jit-pre-opt and jit-post-opt: it seems that I end up either giving misleading jit-pre-opt or messing up inlining edge cases (or add lots of code to do both, which is messy). The new commit thus doesn't try to do this: it either makes the IR tests more general or sets YKD_OPT=0. Since the lang_tests are mostly testing aot_to_hir I think the latter is OK.

While doing this, it turned out -- to my surprise! -- that we're also encountering the const case in j2 (which we didn't in jitc_yk, I think because of the worse inlining). So I've had to implement that too.

@Fidget-Spinner If you're OK with this, it will need squashing.

@Fidget-Spinner
Copy link
Contributor

Turning off opt for certain lang tests seem fine to me. Please squash. I will copy my known bits changes to the new branch after merging.

This is more-or-less a port of jitc_yk's `heapvalues` pass, but tidied
up and adjusted for j2. This does make a useful difference to several
benchmarks, but its main use right now is to help me understand what
different kinds of passes might need from the optimiser API.
@ltratt
Copy link
Contributor Author

ltratt commented Dec 27, 2025

Squashed.

@Fidget-Spinner Fidget-Spinner added this pull request to the merge queue Dec 27, 2025
Merged via the queue into ykjit:master with commit a6d90d0 Dec 27, 2025
2 checks passed
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