Rajat Jindal (Jun 09 2024 at 01:12): Rajat Jindal (Jun 09 2024 at 01:12):Hi Randy, i have been following your work on wit-bindgen-go and noticed that you pushed some changes to how imports/exports are flattened.
I was trying those changes and ran into same issue I reported re: when variants had a option<u64> type.
Is that expected right now?
Rajat Jindal (Jun 10 2024 at 15:54):tagging @Randy Reddig for a kind ping. (sorry and thank you).
Randy Reddig (Jun 10 2024 at 15:56):The flattening logic in wit-bindgen-go is currently wrong. The two tracking issues: https://github.com/ydnar/wasm-tools-go/issues/98 and https://github.com/ydnar/wasm-tools-go/issues/97
Randy Reddig (Jun 10 2024 at 15:57):PRs merged toward addressing this issue:
Randy Reddig (Jun 10 2024 at 15:58):TL;DR: I misread the Canonical ABI flattening rules, and expected LLVM to handle all of the flattening.
The challenge is with variant types, which have distinct flattening per-case, which means the problem cannot be delegated to the compiler, since the compiler doesn’t understand the underlying Component Model type system.
Randy Reddig (Jun 10 2024 at 16:00):@Alex Crichton @Luke Wagner my hot take on Canonical ABI flattening rules: if I could vote for one change, it would be to force i32 pointer params any time a function param accepts a non-primitive type, rather than > 16 flattened params.
Alex Crichton (Jun 10 2024 at 16:10):It's possible, but pretty difficult to change at this point. It's not something where we can just flip a switch and have everything be updated because the current binary format is stable enough it needs to be supported for some time now.
Randy Reddig (Jun 10 2024 at 16:46):If MAX_FLAT_PARAMS was redefined as 1, could that be detected at the ABI level when loading a component?
Alex Crichton (Jun 10 2024 at 16:54):I mostly mean to convey that changing the abi is not a trivial thing to do, it's actually quite involved in terms of actually rollling out the change. In that sense if this is somethiing that's inconvenient, but possible, to implement then that's probably best. If it's basically impossible to implement then we need to start pretty soon in figuring out how to change the ABI
Randy Reddig (Jun 10 2024 at 17:00):Sure, understood. I suppose my underlying question was: what drove the choice of MAX_FLAT_PARAMS = 16, coupled with the CABI flattening rules for variant types (and therefore result and a lesser extent option)?
What are the performance goals (minimizing copying, register usage on AOT compiled code), goals for ease of implementation, correctness, etc.? The current spec has 4 distinct code paths:
Rajat Jindal (Jun 10 2024 at 17:01):thank you for your detailed response with mention of issues/pr's.
The challenge is with variant types, which have distinct flattening per-case, which means the problem cannot be delegated to the compiler, since the compiler doesn’t understand the underlying Component Model type system.
to ensure I understood it correctly, the fix is not ready for re-testing right?
Alex Crichton (Jun 10 2024 at 17:07):MAX_FLAT_PARAMS was set to 16 since it's a limit for the entire function, not per-parameter. If it were set to 1 then component model functions would be able to take a single parameter as an actual parameter and anything more than that would be forced to go through memory. Parameters are also special because if they become indirect then the realloc ABI option is required and it was desired that for most common usage that would not be required. That's both an efficiency thing and a "let's try to not depend on everything everywhere" thing.
The current spec was written in such a way that of the 4 paths you mention there's in theory only two which apply the same algorithm. Params/results have a different maximum (params 16, results 1). Given a WIT signature the types are all flattened and then if it's larger than the maximum it becomes indirect.
FWIW this is sort of why all the infrastructure in wit-bindgen-core exists. It helps abstract this all away across each language so each language only has to deal with various specifics like "load this 32-bit integer" etc.
Last updated: Nov 22 2024 at 17:03 UTC