Check HFA alignment for valuetype fields #106099
Conversation
ghost
added
the
|
No pipelines are associated with this pull request. |
|
/azp list |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@steveisok Who is working on fixing the crossgen2 outerloop? It appears that determinism in the crossgen2 compiler is broken for x64 to x86 builds, and there is a fundamental problem with composite R2R causing lock order violations? I don't see bugs filed here. @kg I'd like to see a cleaner crossgen2 outerloop pass than what you're seeing here. This lock order problem is NOT related to what you are doing, but if you could modify |
|
cc @dotnet/jit-contrib if anyone can assist here |
|
I spent a while figuring out how to run the ARM32 JIT tests and was able to confirm that they don't pass, but the necessary steps to iterate on it are pretty time consuming (>1 hr per iteration), and I don't really know enough about ARM or the JIT to make quick progress here. So it would be awesome if someone could help by digging into the ARM issue, even if they don't have time to fix it. I mentioned this in an email but essentially, it looks like on ARM32 specifically, the HFA argument is passed incorrectly. The HFA return value works correctly, and on ARM64 both HFA arguments and return values appear to work. |
|
Also re being productive on arm32, I have had pretty decent luck cross-building from an x64 linux host (via docker) and then mounting the repo + artifacts via SSHFS on an arm64 host (RPI4 in my case) and then running arm32 under docker there ... then if you need to rebuild anything then there's no need to copy files around. And I remote to the boxes via VS code so can direct it all from the same machine. The only real pain is that debugging requires a specially built lldb that @janvorli created and getting the right flavor of the lldb SOS plugin for that lldb takes a bit of care. |
|
Added some instrumentation into morph.cpp locally to examine the arg ABI info for the call into native code: printf(
"%s.%s arg[%u] numRegs=%u byteOffset=%u byteSize=%u argType=%s passedByRef=%u hfaElemKind=%u IsSplitAcrossRegistersAndStack=%u\n",
comp->info.compClassName, comp->info.compMethodName,
GetIndex(&arg),
arg.AbiInfo.NumRegs,
arg.AbiInfo.ByteOffset,
arg.AbiInfo.ByteSize,
varTypeName(arg.AbiInfo.ArgType),
(unsigned)arg.AbiInfo.PassedByRef,
(unsigned)arg.AbiInfo.GetHfaElemKind(),
abiInfo.IsSplitAcrossRegistersAndStack()
);Based on what godbolt arm32 clang generates for the target function, the numRegs looks suspect. the clang code appears to expect the struct to be passed in |
|
Dumped the segments for the argument and I'm not sure how to interpret them: The first argument being an int in register 0 makes sense if it's the return address in r0. but then it's weird that the 2nd argument has two int segments in r2/r3 and then a stack segment. why isn't r1 being used? I would expect to see four segments here if I understand this system right, first 3 for r1/r2/r3 and then a stack segment. i'm not sure what happened to r1 here. |
|
I feel like it's probably here, but I'm not sure how to confirm it or what the fix would be: runtime/src/coreclr/jit/targetarm.cpp Lines 80 to 85 in c4cc794 |
|
This issue was a bug in the C side of the test. According to the MS version of the arm32 ABI:
In order to approximate the StructLayout with explicit offsets, I had done This does raise the question of whether the natural alignment of the C# struct should still be 8 bytes even though it has a field with a weird offset. I don't know how to answer the question, but it seems like what we do right now is consistent across targets and can be reproduced in C. The question is what the real-world scenario(s) for this sort of struct look like, I think. |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Looks like the windows x86 crossgen2 comparison is failing: |
|
The R2R composite lanes appear to be failing with the same error across linux/osx on both arm64 and x64, so I don't think my HFA changes can be responsible: |
|
Another category of failure on the R2R lanes that looks unrelated: |
The test case I added in this PR should be good enough, it just happens that I saw it cause a crossgen failure before at least once. All I need is a code review if you feel comfortable doing that |
|
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr r2r-extra |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
All failures seem to be #118006 |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #80393 related to HFA (Homogeneous Float Aggregate) alignment validation in CoreCLR. The change ensures that HFA classification correctly rejects structs with misaligned value type fields according to ARM32/ARM64 ABI requirements.
- Adds alignment validation for HFA value type fields in the EEClass::CheckForHFA() method
- Introduces a comprehensive test case that validates HFA behavior with deliberately misaligned struct fields
- Includes documentation for testing ARM32 code on ARM64 hardware
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/vm/class.cpp | Adds alignment checks for HFA value type fields based on their element type |
| src/tests/JIT/Directed/StructABI/StructABI.cs | Adds test structs and wrapper function to validate HFA alignment behavior |
| src/tests/JIT/Directed/StructABI/StructABI.c | Implements native C function to test HFA parameter passing with misaligned fields |
| docs/workflow/testing/coreclr/running-arm32-tests.md | Documents process for running ARM32 tests on ARM64 hardware |
|
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr r2r-extra |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Should fix #80393
I've never really touched CoreCLR so I'm not certain this is right.