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

Use table.fill in externref-xform (TODO) #3446

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

lukaslihotzki
Copy link
Contributor

No description provided.

@lukaslihotzki lukaslihotzki force-pushed the table-fill branch 2 times, most recently from 0ccb9bc to 9740e87 Compare May 22, 2023 15:19
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

First off: this is amazing, thank you!

As far as I know the reference-type proposal is not a requirements for the atomic proposal, so either we will have to adjust the documentation or gate it.

I would prefer to gate it, which should be possible.

Will pull in alexcrichton when we are ready to review this, I don't feel comfortable enough with Wasm's instruction set to approve this.

@lukaslihotzki
Copy link
Contributor Author

I don't understand where gating makes sense. The table.set and table.fill instructions are both newly defined in the reference-types proposal, so I don't see any additional requirements. If externref_stack == 0, neither of those instructions is used anyway.

@daxpedda
Copy link
Collaborator

Ah, apologies! For some reason I thought we are in threads-xform 🤣.
@alexcrichton if you wouldn't mind taking a look at this.

@alexcrichton
Copy link
Contributor

Yes I believe that this is correct but I'm also not sure whether it's worth doing. For small sizes it may be better to have the currently unrolled loop perf-wise and it may only be advantageous for larger sizes to use table.fill. I'm not entirely sure though in that I haven't benchmarked any wasm engine to see the difference.

@daxpedda
Copy link
Collaborator

daxpedda commented May 25, 2023

I would have argued that loop unrolling is a compilers job when it's applying optimizations, on the other hand this is not something I really know much about.

@lukaslihotzki would you mind researching this a bit:

  • Benchmark it.
  • Find out if the LLVM or wasm-opt unrolls table.fill.
  • See how it would make sense to proceed here in your opinion.

Thank you!

@daxpedda daxpedda self-assigned this May 25, 2023
@daxpedda daxpedda added the needs discussion Requires further discussion label May 25, 2023
@daxpedda
Copy link
Collaborator

daxpedda commented Jun 19, 2024

I'm going to go ahead and merge this.

I would argue that the current implementation is premature optimization as long as we don't have any data and the change proposed here is the straightforward way to go about it.

@daxpedda daxpedda merged commit 617167f into rustwasm:main Jun 19, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants