Wasmtime GitHub notifications bot (Jul 18 2024 at 20:41): Wasmtime GitHub notifications bot (Jul 18 2024 at 20:41):fitzgen opened PR #8974 from fitzgen:stack-slots-and-alias-analysis to bytecodealliance:main:
And do our usual alias analysis stuff on it.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jul 18 2024 at 20:41):fitzgen requested elliottt for a review on PR #8974.
Wasmtime GitHub notifications bot (Jul 18 2024 at 20:41):fitzgen requested wasmtime-compiler-reviewers for a review on PR #8974.
Wasmtime GitHub notifications bot (Jul 18 2024 at 20:55):fitzgen updated PR #8974.
Wasmtime GitHub notifications bot (Jul 18 2024 at 20:55):If you have a store with no alias flags and a load with the stack alias flag will they be considered as potentially aliasing? If not, this PR makes it impossible to combine loads and stores on the output of stack_addr with stack_load and stack_store.
Should the docs for stack_load and stack_store be updated to indicate that the stack alias region is used?
Wasmtime GitHub notifications bot (Jul 18 2024 at 20:55):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Jul 18 2024 at 20:57):fitzgen commented on PR #8974:
If you have a store with no alias flags and a load with the stack alias flag will they be considered as potentially aliasing? If not, this PR makes it impossible to combine loads and stores on the output of stack_addr with stack_load and stack_store.
Should the docs for stack_load and stack_store be updated to indicate that the stack alias region is used?
You can still arrange for them to be combined, but you have to specify the
stackalias region yourself on the manually-legalized loads and stores.I'll update the docs tho, good point.
Wasmtime GitHub notifications bot (Jul 18 2024 at 21:00):You can still arrange for them to be combined, but you have to specify the stack alias region yourself on the manually-legalized loads and stores.
That basically means I can no longer use stack_load and stack_store in cg_clif. A stack_addr result can be passed to arbitrary functions which don't know whether the pointer argument is on the stack or the heap. Maybe you could only apply the stack alias region if there is no stack_addr on the same stack slot anywhere inside the function?
Wasmtime GitHub notifications bot (Jul 18 2024 at 21:03):This would also be the first UB case where the generator of the IR doesn't explicitly opt in. UB for regular loads and stores is fully opt in. Even being able to access the pointer target is not UB if you don't set the right flag to indicate that the load/store must never fail.
Wasmtime GitHub notifications bot (Jul 18 2024 at 21:04):cfallin commented on PR #8974:
Perhaps we could accept an alias region on the stack-slot declaration which is inherited by the
stack_addr? That way, by default, we retain today's behavior, but producers that control aliasing more tightly (e.g. ensure the pointer doesn't escape) could get better optimization.
Wasmtime GitHub notifications bot (Jul 18 2024 at 21:05):fitzgen closed without merge PR #8974.
Wasmtime GitHub notifications bot (Jul 18 2024 at 21:05):fitzgen commented on PR #8974:
Okay, on reflection let's skip this for the time being. Wasmtime actually has similar issues where we would need to start threading memflags through various places.
Last updated: Oct 23 2024 at 20:03 UTC