c_variadic: provide our own va_arg implementation for more targets
#150094
Conversation
|
|
| Arch::AmdGpu => emit_ptr_va_arg( | ||
| bx, | ||
| addr, | ||
| target_ty, | ||
| PassMode::Direct, | ||
| SlotSize::Bytes4, | ||
| AllowHigherAlign::No, | ||
| ForceRightAdjust::No, | ||
| ), |
There was a problem hiding this comment.
There was a problem hiding this comment.
just making sure @Flakebi sees this, since we had one platform weirdness and the idea of varargs on a GPU at least feels inherently fishy, but maybe it's like... fine actually? feels like a bad idea performance wise, but
There was a problem hiding this comment.
Comparing the Rust and clang implementation, this looks fine (thanks for checking).
I wanted to say I never tried varargs on GPUs but I did use printf, so… (GPUs inline everything anyway, so as long as it’s inlined before instruction selection it’s not a problem performance wise)
| Arch::Nvptx64 => emit_ptr_va_arg( | ||
| bx, | ||
| addr, | ||
| target_ty, | ||
| PassMode::Direct, | ||
| SlotSize::Bytes1, | ||
| AllowHigherAlign::Yes, | ||
| ForceRightAdjust::No, | ||
| ), |
There was a problem hiding this comment.
There was a problem hiding this comment.
cc @kjetilkjeka for similar reasons: just letting you know about this if you have a comment.
| Arch::Wasm32 | Arch::Wasm64 => emit_ptr_va_arg( | ||
| bx, | ||
| addr, | ||
| target_ty, | ||
| if layout.is_aggregate() || layout.is_zst() || layout.is_1zst() { | ||
| PassMode::Indirect | ||
| } else { | ||
| PassMode::Direct | ||
| }, | ||
| SlotSize::Bytes4, | ||
| AllowHigherAlign::Yes, | ||
| ForceRightAdjust::No, | ||
| ), |
There was a problem hiding this comment.
curiously the slot size is always 4, independent of whether wasm32 or wasm64 is used. I suppose that may change in the future if/when wasm64 gets more attention.
There was a problem hiding this comment.
Hrrm. Should we make this work on wasm64 at all, then, if we expect a change? I don't want to put @alexcrichton and @daxpedda through another round of wasm ABI grief, even if variable arguments are more niche.
There was a problem hiding this comment.
I don't see anything special in the Wasm tool convention saying that pointers for Varargs are suddenly 32-bit, so this is a bug in LLVM. I've encountered similar bugs around Wasm64 in LLVM already so this tracks.
Alex should confirm though, never touched the internals of Vararg in my life before. In the meantime I will open a PR in LLVM where target maintainers can confirm that as well.
There was a problem hiding this comment.
Nice, thanks.
Assuming that makes it into llvm 22 (i.e. is merged before late January), what should we do on the rust side until then?
There was a problem hiding this comment.
I think this is best answered by the compiler team but just wanted to note that wasm64-unknown-unknown is not stable so maybe this doesn't have to be a blocker for anything. I'm also happy to push for backporting this to a LLVM patch version if it helps.
There was a problem hiding this comment.
just wanted to note that
wasm64-unknown-unknownis not stable so maybe this doesn't have to be a blocker for anything.
It is not a blocker, just a matter of how we do the best thing we can right now and ensure that we circle back to this when the LLVM fix lands and/or when wasm64 becomes more official.
I'm also happy to push for backporting this to a LLVM patch version if it helps.
If this can make it into LLVM 22 then I think that's good enough.
There was a problem hiding this comment.
Hmm. I'm currently on the side of "fatal error or ICE if someone uses wasm64-u-u with varargs and it reaches this actual codegen site" but can be persuaded this is a bad idea.
| Arch::CSky => emit_ptr_va_arg( | ||
| bx, | ||
| addr, | ||
| target_ty, | ||
| PassMode::Direct, | ||
| SlotSize::Bytes4, | ||
| AllowHigherAlign::Yes, | ||
| ForceRightAdjust::No, | ||
| ), |
There was a problem hiding this comment.
There was a problem hiding this comment.
cc @Dirreke if there's anything of particular interest here.
|
☔ The latest upstream changes (presumably #150089) made this pull request unmergeable. Please resolve the merge conflicts. |
1c516dc to
9d0af69
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
tracking issue: #44930
Provide our own implementations in order to guarantee the behavior of
va_arg. We will only be able to stabilizec_variadicon targets where we know and guarantee the properties ofva_arg.r? workingjubilee