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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukaslihotzki
Copy link
Contributor

No description provided.

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
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.

None yet

3 participants