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

[wasm64] Enable table64 lowering #21992

Merged
merged 1 commit into from
May 24, 2024
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 23, 2024

This change prepares for the LLVM change which actually enables the use of table64 in the output:
llvm/llvm-project#92042

This change does several things:

  • Enables the table64 lowering pass in binaryen
  • Fixes the signatures of the dynCall_xxx functions (which take a pointer as their first argument). Previously we got away with using i32 here since functions pointers happened to be i32.
  • Forces the export or __table_base32. Previously llvm would generate references to this symbol which meant it would get pulled in via normal mechanism. Now the references are generate by a post-link binaryen pass, so we need to explicitly export it.

@sbc100 sbc100 requested review from kripken and dschuff and removed request for dschuff May 23, 2024 19:16
@sbc100 sbc100 enabled auto-merge (squash) May 23, 2024 20:57
This change prepares for the LLVM change which actually enables
the use of table64 in the output:
  llvm/llvm-project#92042
@sbc100 sbc100 force-pushed the table64_lowering branch from ad9fef3 to 6ec5cff Compare May 23, 2024 22:22
@sbc100 sbc100 requested a review from kripken May 23, 2024 22:25
@sbc100
Copy link
Collaborator Author

sbc100 commented May 23, 2024

@dschuff @kripken, I'd love to get this landed today if possible since the LLVM side of this is currently blocked on this.

@dschuff
Copy link
Member

dschuff commented May 23, 2024

Can you write a note in the commit message about the other stuff that this PR does (table32 export , the dyncall signature thing) and why?

@sbc100
Copy link
Collaborator Author

sbc100 commented May 23, 2024

Can you write a note in the commit message about the other stuff that this PR does (table32 export , the dyncall signature thing) and why?

Done!

@dschuff
Copy link
Member

dschuff commented May 24, 2024

Thanks!
For the dyncall signatures, why are they not being generated with 'p' params in the first place (i.e. why do they need to be fixed up here?)

Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

Change LGTM though, we will be modifying the dyncall part again soon anyway.

@sbc100 sbc100 merged commit bd5fb74 into emscripten-core:main May 24, 2024
29 checks passed
@sbc100 sbc100 deleted the table64_lowering branch May 24, 2024 00:15
@sbc100
Copy link
Collaborator Author

sbc100 commented May 24, 2024

Thanks! For the dyncall signatures, why are they not being generated with 'p' params in the first place (i.e. why do they need to be fixed up here?)

The first argument to the dyncall function is a table index. With table32 the first argument to the dyncall function was always i32. With table64 we start seeing the first argument being i64 and even after lowering the ABI is still i64 (with a wrap injected by the lowering pass on the call_indirect).

@dschuff
Copy link
Member

dschuff commented May 24, 2024

Right, but I guess my question was, why don't we know whether we're using table32 or table64 at the time the signatures are generated?

@sbc100
Copy link
Collaborator Author

sbc100 commented May 24, 2024

Right, but I guess my question was, why don't we know whether we're using table32 or table64 at the time the signatures are generated?

Yes we do know in binaryen, and we generate the right signatures in binaryen these days. This was enabled in WebAssembly/binaryen#6604.

The differences was that before we would always generated dynCalls with i32 for the first argument even for memory64.. mostly because that was the easiest thing to do and pointers in JS are numbers anyway.

Once I landed WebAssembly/binaryen#6604 then emscripten now has to deal with two different types of dynCalls onces with i32 arg0 and onces with i64 arg0 (under memory64). This code here handles the latter case. Because we just ignored the issue by always generating wasm32-style dyncalls that just happened to work fine.

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.

3 participants