Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-clang

Author: None (daxpedda)

diff --git a/clang/lib/CodeGen/Targets/WebAssembly.cpp b/clang/lib/CodeGen/Targets/WebAssembly.cpp
index ebe996a4edd8d..ce97884fa57c4 100644
--- a/clang/lib/CodeGen/Targets/WebAssembly.cpp
+++ b/clang/lib/CodeGen/Targets/WebAssembly.cpp
@@ -160,10 +160,11 @@ RValue WebAssemblyABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
   bool IsIndirect = isAggregateTypeForABI(Ty) &&
                     !isEmptyRecord(getContext(), Ty, true) &&
                     !isSingleElementStruct(Ty, getContext());
-  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, IsIndirect,
-                          getContext().getTypeInfoInChars(Ty),
-                          CharUnits::fromQuantity(4),
-                          /*AllowHigherAlign=*/true, Slot);
+  return emitVoidPtrVAArg(
+      CGF, VAListAddr, Ty, IsIndirect, getContext().getTypeInfoInChars(Ty),
+      CharUnits::fromQuantity(
+          getContext().getTargetInfo().getTriple().isArch64Bit() ? 8 : 4),
+      /*AllowHigherAlign=*/true, Slot);
 }
 
 std::unique_ptr<TargetCodeGenInfo>

I believe this change is incomplete, consider

#![feature(c_variadic)]

#[unsafe(no_mangle)]
unsafe extern "C" fn foo(x: ...) -> u32 {
    let mut x = core::hint::black_box(x);
    let _ = unsafe { x.arg::<u32>() };
    unsafe { x.arg::<u32>() }
}

#[unsafe(no_mangle)]
unsafe extern "C" fn bar() -> u32 {
    foo(123u32, 456u32)
}

This currently produces

.type	bar,@function
bar:
	.functype	bar () -> (i32)
	.local  	i64, i32
	global.get __stack_pointer
	i64.const 16
	i64.sub 
	local.tee 0
	global.set __stack_pointer
	local.get 0
	i64.const 1958505087099
	i64.store 0
	local.get 0
	call foo
	local.set 1
	local.get 0
	i64.const 16
	i64.add 
	global.set __stack_pointer
	local.get 1
	end_function

where 1958505087099 is 123 | 456 << 32. In other words, the call itself passes the arguments with a slot size of 4 as well.

I believe that the slot size is mostly relevant for aligning the values in the variable arguments list. That might be less important for wasm, although the alignment of 64-bit integers is 8 on the vast majority of host targets that might run a wasm interpreter.

Based on #175102 (comment), the title is confusing here. What is being adjusted is the slot size on wasm64 (from 4 to 8) to match other 64-bit architectures.

Thanks for the clarification. So we are changing the minimum size (and alignment with it) of the slots in the vararg buffer, meaning that the slot will always be at least 8 bytes. Previously it was always at least 4, but could be e.g. 8 for i64/f64. In wasm64 there would of course be many more i64s in practice. But, I'm not really understanding where the bug is, or what's causing a problem with Rust. Or, for that matter, what the advantage would be to a larger slot size. It does mean that those now-more-prevalent pointer-sized values will always be aligned on the stack (assuming we are also ensuring that the buffer itself is aligned), allowing aligned loads and stores at the cost of a larger buffer. Is that it? You mentioned consistency with other architectures, but most architectures that I'm aware of (at least, machine architectures) try to use the same calling convention for vararg as for regular calls, so wasm is a bit weird there.
Again, not that I'm necessarily against this, just trying to understand what the underlying issue is.

To me, there is no actual issue here: it is purely a question of whether a slot size of 4 was a deliberate decision, it very much looks like it wasn't.

A slot size of 4 is perfectly functional, however the 64-bit architectures that I've seen all use a slot size of 8, presumably as you say so that 64-bit values are aligned. So wasm64 can pick a slot size of either 4 or 8, I don't really have strong opinions on that, so long as it is deliberate. If a slot size of 4 is kept I think that's at least worthy of a comment.

OK, fair enough. I just wanted to make sure I understood the situation since the comments at rust-lang/rust#150094 (comment) seemed to suggest there was a bug in LLVM.

It looks like a bug on first glance, and there probably have been many similar issues in the past where someone forgot to update a 4 to an 8 or an i32 to a i64 for wasm64.

In this particular case using a slot size of 4 can actually work, so perhaps this PR was over-eager. From the rust side I think all we'd like to see is a deliberate choice.

OK, I looked at this a little more, and I think I'm understanding the situation better.

It sounds like you are considering that each vararg is passed in its own pointer-sized slot in the buffer. But I think the "correct" way to think about it (at least, the way I thought about it when I wrote it originally) is that each item is placed in the buffer at the next available offset according to its ABI size and alignment (the latter of which can be overridden explicitly).
The ABI sort of hints at that, but it's definitely a bit vague and needs to be improved.
However all this happens after legalization (i.e. types are promoted to the sizes they'd use for passing as regular arguments), so nothing is smaller than i32. That means that until now everything other than i64s (and larger, more on that in a minute) just goes in a pointer-sized slot, so it's effectively the same for small types. i64s obviously get a bigger slot, naturally aligned. I also now understand why this PR is framed as "fixing the pointer width", since most things are getting a pointer-sized slot.

But, given that even in the wasm32 ABI, i64s are not exactly rare (and already have the effect of taking a "double slot"), I don't think it really makes sense to think of everything in terms of fixed pointer-sized slots. That would suggest that the wasm64 ABI should continue to be framed in terms of the legalized type of each argument, rather than always rounding up to a consistent size. I just don't really see an advantage to always rounding up, given that it will take more stack size, and won't buy us full consistency of slot size (and each item will remain at least naturally aligned in any case).

Having said all that, this did reveal some oddness (possibly a bug) with how vectors and fp128 (or presumably anything that's broken up and passed in multiple arguments) are passed. I need to look at that more closely. It's relevant to the details of your change to WebAssemblyISelLowering, because Flags.getNonZeroOrigAlign() returns the aligment of the overall vector or original type for the first argument, and 1 for the rest; whereas your substitute of the pointer alignment ends up being the minimum slot size. WIth the current lowering of separate slots for each element, it's not necessary to over-align the first element (and I'm not sure that's actually working on the va_arg side), I will have to check on that.