Wasmtime GitHub notifications bot (Aug 13 2020 at 04:27): Wasmtime GitHub notifications bot (Aug 13 2020 at 04:27):github-actions[bot] commented on Issue #2128:
Subscribe to Label Action
cc @bnjbvr
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst"Thus the following users have been cc'd because of the following labels:
- bnjbvr: cranelift
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Aug 14 2020 at 21:38):cfallin commented on Issue #2128:
Thanks for the detailed review!
Wasmtime GitHub notifications bot (Aug 17 2020 at 13:11):uweigand commented on Issue #2128:
I just noticed that the new interface makes it difficult to implement the IBM Z ABI (which I already had working with the old interface), because it is now (it seems) overly prescriptive in how the stack has to be layed out.
Specifically, the split of gen_prologue into gen_prologue_frame_setup and gen_clobber_save is not a good idea on Z. This seems to enforce the idea that clobbered registers must be saved below stackslots and spillslots. In the Z ABI, however, the caller provides a register save area (via a biased stack pointer) on function entry that the callee can (and should) use to save clobbered registers. So the sequence is usually: 1) save clobbers (via a single STORE MULTIPLE instruction, which also includes the old SP value, so that the LOAD MULTIPLE in the epilog will simultaneously restore clobbers and de-allocate the stack frame) 2) allocate stack frame by decrementing SP.
This doesn't work with the new interface any more. In gen_prologue_frame_setup I cannot save the clobbers, because I don't even get the list here. But in gen_clobber_save, common code has already allocated the stack frame, so I cannot save the original SP any more.
Some of the other refactoring in the patch makes sense also on Z, but I do believe a generic "gen_prologue" interface that allows the target freedom in how to allocate its stack frame is still necessary to handle a variety of targets.
(As an aside, I'm not sure that saving clobbers below stackslots and spillslots is a good idea on any platform; neither GCC nor LLVM do it this way. In general, you want the frequently accessed areas at short distances off the stack pointer so they can be accessed with small displacement instructions, on ISAs where this is an issue. The clobbered register save areas are usually the least frequently accessed slots ...)
Wasmtime GitHub notifications bot (Aug 17 2020 at 13:27):bjorn3 commented on Issue #2128:
The
ABIBodytrait is not changed. This PR only introduces a newABIBodyImplstruct that can help with getting anABIBodyimpl when the ABI follows certain rules. If it doesn't follow these rules, you can still manually implementABIBody.
Wasmtime GitHub notifications bot (Aug 17 2020 at 14:08):uweigand commented on Issue #2128:
Yes, I got that. Still, it would be nice to be able to make use of the new ABIBodyImpl, since a lot of that refactoring is in fact useful on Z as well. It just goes a little bit too far with the prolog/epilog details.
Wasmtime GitHub notifications bot (Aug 17 2020 at 15:47):cfallin commented on Issue #2128:
@uweigand Thanks for the feedback -- indeed, I'm happy to refactor things as needed to support other architectures. (The layout issue you note with clobbers is also preexisting and had to do with constraints on what we know at what time -- though I think we can do better now that we track nominal-SP.)
In more detail, this was mostly motivated by the fact that the x64 ABI had copied most of the AArch64 ABI verbatim, so this was a long-overdue refactor to fix that particular problem. As additional architectures come online, we can shift things around to be more flexible!
Wasmtime GitHub notifications bot (Aug 17 2020 at 19:59):uweigand commented on Issue #2128:
@cfallin I've now managed to move the Z backend to the new ABI helper while still generating the same code. I've had to change a number of things:
- Pass call_conv and clobbers to the gen_prologue_frame_setup and gen_epilogue_frame_restore helpers as well.
- In addition, pass the frame size to gen_epilogue_frame_restore (this could possibly be avoided, but it seems logical to have the size available here, given that this routine is responsible for deallocating the frame).
- Allow the machine to specify a different stack alignment than 16.
- Fix a bug in the argument extension code.
As to the that last bug in argument extension: The current code, when handling a ABIArg::Stack argument requiring extension, will perform an in-register extension of the argument value to I64, and then generate a store using the original type. This means the extension is in fact just ignored ... To fix this, the store needs to happen in I64 instead of the original type. This bug is present both in gen_copy_reg_to_retval and emit_copy_reg_to_arg.
Wasmtime GitHub notifications bot (Aug 17 2020 at 20:24):cfallin commented on Issue #2128:
Thanks! I'm happy to review a PR that does all of that, if you'd like. I'll plan to fix the argument-extension bug right now. (In general, for small design changes like passing additional args, I don't really think we need to have a design discussion first; I can just r+ a PR right away.)
Wasmtime GitHub notifications bot (Nov 02 2020 at 12:28):uweigand commented on Issue #2128:
Thanks! I'm happy to review a PR that does all of that, if you'd like. I'll plan to fix the argument-extension bug right now. (In general, for small design changes like passing additional args, I don't really think we need to have a design discussion first; I can just r+ a PR right away.)
@cfallin I've now opened two PRs:
https://github.com/bytecodealliance/wasmtime/pull/2345
https://github.com/bytecodealliance/wasmtime/pull/2346The other changes discussed above are no longer necessary in the current code base.
Last updated: Dec 23 2024 at 12:05 UTC