Cranelift: Remove ABICallee trait
#4701
Conversation
github-actions
bot
added
cranelift
There was a problem hiding this comment.
Always nice to not only get rid of dynamic dispatch but also heap indirection! This is cool.
I spotted a couple comments that could use a little help, but this looks good to me. I assume Chris should still take a look though.
Naming conventions are hard. AbiCallee looks weird to me. But 🤷
|
I thought this trait existed because some architectures may have an ABI sufficiently different from the norm that ABICalleeImpl would not be able to implement it. |
There was a problem hiding this comment.
Thanks for tackling this! I'm happy to see things simplified here. LGTM overall, just one slightly bikesheddy comment below.
To address a few points in discussion:
I thought this trait existed because some architectures may have an ABI sufficiently different from the norm that ABICalleeImpl would not be able to implement it.
The trait initially existed because each backend implemented it separately; there was no shared "vanilla ABI" code. Eventually I refactored things to share that (this was in #2128 and #2142 for aarch64 and x64 respectively; and s390x never had its own impl, it has always used abi_impl EDIT: reading the first PR further I'm reminded s390x did have its own backend, out of tree, but moved to abi_impl before merging here IIRC.). We kept the trait around because, as you suggest, there was a thought that other machines might need very different implementations.
But in practice, (i) this trait separation adds complexity and hinders refactoring (@fitzgen was hitting some issues recently, motivating this PR), and (ii) we've actually managed to make the shared abi_impl code pretty flexible, by adding hooks to the ABIMachineSpec. I'm confident that we can tweak things as needed to fit most reasonable architectures -- e.g., the in-progress riscv64 backend uses it with no problem -- and for truly different architectures, we can update the design when we understand the requirements better.
Naming conventions are hard. AbiCallee looks weird to me. But 🤷
Indeed, we spent a bunch of discussion on this two years ago, actually; it was originally some other name that escapes me at the moment (ABIBody maybe?) and there was a suggestion to name the traits in terms of the role that they represent. So ABICallee is the callee side (receiving arguments, doing prologues and epilogues) and ABICaller is the caller side (setting up arguments, emitting a call instruction, noting clobbers as side-effects).
It has only one implementation: the `ABICalleeImpl` struct. By using that directly we can avoid unnecessary layers of generics and abstractions as well as a couple `Box`es that were previously putting the single implementation into a `Box<dyn>`.
fitzgen
force-pushed
the
no-more-abi-callee-trait
branch
from
8e249d2 to
445be8c
Compare
| @@ -65,7 +65,7 @@ fn saved_reg_stack_size( | |||
|
|
|||
| /// AArch64-specific ABI behavior. This struct just serves as an implementation | |||
| /// point for the trait; it is never actually instantiated. | |||
| pub(crate) struct AArch64MachineDeps; | |||
| pub struct AArch64MachineDeps; | |||
There was a problem hiding this comment.
Because it is public now that there is the MachInst::ABIMachineSpec associated type.
No description provided.