fix is_single_fp_element for s390x and x86 - #161987
Conversation
rustbot
added
A-LLVM
| // Match GCC and Clang in allowing trailing padding. This does appear to violate the | ||
| // specification, but is well-established in both compilers. | ||
| // | ||
| // e.g. `#[repr(C, align(4))] struct Foo(f16)` is passed as `Reg::f32()`. |
There was a problem hiding this comment.
I've done some further research into this. AFAICT it seems that Clang and GCC, while they do use larger loads/stores than needed, do (coincidentally?) end up doing the right thing with the full struct size is <= 8 bytes (specifically, loading e.g. a 8 byte struct as a f16, when the struct is a f16 followed by 6 bytes of padding, will mean the f16 ends up in the right place in the register anyway). The only difference between Clang/GCC's behaviour and the ABI spec is that structs which contain a single f16/f32/f64 that are larger than 8 bytes won't get passed in a floating point register. In summary, the problem is that Clang/GCC don't ignore the trailing padding when they should.
There was a problem hiding this comment.
cc @uweigand in general on this PR but here in particular
There was a problem hiding this comment.
(Re-reading the spec again, it does say "[...] load the argument value left-aligned into floating-point register [...]", which is probably why GCC/Clang load all the bytes instead of just the bytes of the floating-point value (of course it doesn't matter whether the padding bytes are loaded or not as they're padding, except when there's so much padding the full size of the values doesn't fit in the register). I'm guessing overaligned structs weren't considered one way or another when writing this part of the spec)
5681e66 to
50861ec
Compare
50861ec to
5867f93
Compare
| // On s390x trailing padding is allowed in practice by GCC and Clang, | ||
| // although this violates the specification. |
There was a problem hiding this comment.
After giving this a lot of thought, I don't think this wording is correct. Clang/GCC allowing trailing padding isn't the issue here, the issue is that Clang/GCC don't allow trailing padding if it makes the struct larger than 8 bytes, whereas the ABI spec doesn't give any limit on the size of the struct. For structs <= 8 bytes both Clang/GCC's behaviour and my interpretation of the ABI spec are the same in practice (Clang/GCC use suboptimally large loads/stores, but end up with the same result).
There was a problem hiding this comment.
I've tried to rephrase it
5867f93 to
2172749
Compare
2172749 to
9ae7ff3
Compare
|
I'll leave it as a draft because I'm not really sure who to assign here, and I'd like some feedback from the s390x target maintainer anyway. |
| { | ||
| // Contrary to X86, trailing padding is allowed on s390x. | ||
|
|
||
| layout = layout.peel_transparent_wrappers(cx); |
There was a problem hiding this comment.
TIL that peel_transparent_wrappers exists. However, its logic can only work for non-1-ZST types. The function should be renamed to reflect that as people might think it handles repr(transparent) for everything, and the doc comment of peel_transparent_wrappers should be clarified to call this out.
| if is_single_fp_element(arg.layout, cx) { | ||
| // Match GCC and Clang by explicitly passing padding, even though their behavior violates | ||
| // (our reading of) the specification, which says that: | ||
| // | ||
| // > Structures equivalent to a floating point type are passed in floating point registers. | ||
| // > A structure is equivalent to a floating point type if and only if it has exactly one | ||
| // > member, which is either of floating point type of itself a structure equivalent to a | ||
| // > floating point type. | ||
| // | ||
| // When the alignment is at most 8 but still overaligns the element, our implementation | ||
| // (matching GCC and Clang) is compliant but does require suboptimally large loads and | ||
| // stores. | ||
| // | ||
| // When the alignment is higher than 8, we passed the argument indirectly, which violates | ||
| // the specification but is consistent with GCC and Clang. |
4 participants
In #161950 (comment) we discovered that the old
is_single_fp_elementis incorrect in a number of ways.f16orf128So, in practice each target does something slightly different here, and I've split the function into two.
cc @beetrees