winch: Implement aarch64 call, trapz, address_at_vmctx #9751
Conversation
MarinPostma
requested review from
abrown and
fitzgen
and removed request for
a team
MarinPostma
force-pushed
the
aarch64-call
branch
from
2913248 to
31436c5
Compare
|
I can help taking a look at this one. |
Subscribe to Label Action
This issue or pull request has been labeled: "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This commit aims to improve the consistency of call emission in Winch. Prior to this commit, the calling convention at Winch's assembler layer was hardcoded. Even though the calling convention invariants are correctly handled early on in the code generation process and this has no effect on the generated code, it could lead to subtle bugs if Cranelift's emission infrastructure changes. It could also be confusing when trying to implement call instrutions for other backends. This change is motivated by some of the questions in bytecodealliance#9751
winch/codegen/src/isa/aarch64/asm.rs
Outdated
| @@ -724,6 +723,14 @@ impl Assembler { | |||
| }); | |||
| } | |||
|
|
|||
| /// Trap if rn is 0 | |||
There was a problem hiding this comment.
| /// Trap if rn is 0 | |
| /// Trap if `rn` is zero. |
There was a problem hiding this comment.
fixed in f1a56f1
winch/codegen/src/isa/aarch64/asm.rs
Outdated
|
|
||
| pub fn call_with_name(&mut self, name: UserExternalNameRef) { | ||
| self.emit(Inst::Call { | ||
| info: Box::new(cranelift_codegen::CallInfo::empty( | ||
| ExternalName::user(name), | ||
| CallConv::SystemV, | ||
| )), | ||
| }) | ||
| } | ||
|
|
||
| pub fn call_with_reg(&mut self, callee: Reg) { | ||
| self.emit(Inst::CallInd { | ||
| info: Box::new(cranelift_codegen::CallInfo::empty( | ||
| callee.into(), | ||
| CallConv::SystemV, | ||
| )), | ||
| }) | ||
| } | ||
|
|
||
| pub fn call_with_lib(&mut self, lib: LibCall, dst: Reg) { | ||
| let name = ExternalName::LibCall(lib); | ||
| self.emit(Inst::LoadExtName { | ||
| rd: writable!(dst.into()), | ||
| name: name.into(), | ||
| offset: 0, | ||
| }); | ||
| self.call_with_reg(dst) | ||
| } |
There was a problem hiding this comment.
Would you mind adding some comments to these methods?
There was a problem hiding this comment.
fixed in f1a56f1
winch/codegen/src/isa/aarch64/asm.rs
Outdated
| self.emit(Inst::CallInd { | ||
| info: Box::new(cranelift_codegen::CallInfo::empty( | ||
| callee.into(), | ||
| CallConv::SystemV, |
There was a problem hiding this comment.
Right now the calling convention is hardcoded to SystemV, but maybe we also want to support AppleAarch64?
I see why this might be confusing. Even though the CallConv is hard coded here, there are two things to note:
- All the infrastructure to handle the calling convention differences, is handled very early on in the compilation process in Winch (and in Cranelift as well) e.g., when constructing the signature for the callee, at that point, you'll note that Winch correctly handles the default and apple-aarch64 calling conventions.
- Winch relies on Cranelift's binary emission infrastructure, and in this concrete case, as you can note, you're constructing a
CallInfo::empty(), which signals that none of the other Cranelift-specific invariants must be taken into account when emitting calls (some of them depend on the calling convention that is passed here).
I've opened #9757 to refactor call emission a bit with the hope of making this part more consistent.
There was a problem hiding this comment.
That refactor makes a lot of sense to me 👍
There was a problem hiding this comment.
updated in f4a6cf7
* winch: Track the callee calling convention This commit aims to improve the consistency of call emission in Winch. Prior to this commit, the calling convention at Winch's assembler layer was hardcoded. Even though the calling convention invariants are correctly handled early on in the code generation process and this has no effect on the generated code, it could lead to subtle bugs if Cranelift's emission infrastructure changes. It could also be confusing when trying to implement call instrutions for other backends. This change is motivated by some of the questions in #9751 * Clippy fixes
MarinPostma
force-pushed
the
aarch64-call
branch
2 times, most recently
from
8d4f591 to
af504b5
Compare
|
There are some failures in CI -- I'm trying to figure out if they are related to your changes or not. |
| @@ -0,0 +1,40 @@ | |||
| ;;! target = "aarch64" | |||
| ;;! test = "compile" | |||
There was a problem hiding this comment.
I'm not entirely sure if this is the source of the failure, but here you need to specify
| ;;! test = "compile" | |
| ;;! test = "winch" |
Else this will use Cranelift
There was a problem hiding this comment.
Some submodule update sneaked into one of my commits. That's what's messing with the CI, I'll fix that tommorow
MarinPostma
force-pushed
the
aarch64-call
branch
from
4cc648b to
bbf1501
Compare
|
Alright, I updated the submodule, ci looks good now |
Implement the following MASM instruction for the aarch64 platform:
I initially planned to only implement call, but implementing the tests uncovered the necessity to implement the other two as prerequisite for indirect calls.
Right now the calling convention is hardcoded to SystemV, but maybe we also want to support AppleAarch64?
#8321