[x64][SysV] Classify empty structs for passing like padding #103799
Conversation
The current implementation barred a struct containing empty struct fields from enregistration. This did not match the [System V ABI](https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf) which says "NO_CLASS This class is used as initializer in the algorithms. It will be used for padding and **empty structures** and unions". It also does not match the behavior of GCC & Clang on Linux.
dotnet-policy-service
bot
added
the
community-contribution
|
Could you please add some tests? |
jkotas
added
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib @jakobbotsch |
| // then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT | ||
| // so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass. | ||
| // | ||
| // TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this. |
There was a problem hiding this comment.
Do you plan to fix this TODO in this PR?
There was a problem hiding this comment.
No. It would require going through the JIT like I did in #101796 for RISC-V and fix in many places. Right now my priority is on RISC-V but when I'm done with #101796 I can revisit it. By then @jakobbotsch's ongoing effort to centralize ABI passing infos will probably be more advanced, which will also facilitate fixing x64.
Sure, I'll lift a couple from #101796. EDIT: In fact, I think I can lift the entire test suite and disable Empty8Float tests on System V. |
|
I think the GC\Scenarios\RanCollect\rancollect\rancollect.cmd time-out on Windows x64 is not related to this change (which is System V only). |
| // then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT | ||
| // so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass. | ||
| // | ||
| // TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this. |
There was a problem hiding this comment.
What does the difference between NoClass and something like char[8] end up being? For significant padding (structs with explicit layout) it seems like we have to consider them to be the same or things will end up odd/wrong.
There was a problem hiding this comment.
char[8] is an INTEGER eightbyte which gets assigned an integer register for passing; a NO_CLASS eightbyte doesn't get any register. Passing on the stack is the same, though (NO_CLASS padding does take up space).
There was a problem hiding this comment.
Opened #104098 as I'll need an URL for ActiveIssue in tests in future PRs.
Meanwhile this smaller fix is ready for review.
There was a problem hiding this comment.
Does this fix handle the significant padding cases correctly (i.e. as before this change)?
There was a problem hiding this comment.
It should, the NO_CLASS -> INTEGER eightbyte reclassification is still there. And I didn't see any CLR tests (prio1) go off on Checked build, assuming there are tests for significant padding by now.
|
It looks like there's a bunch of asm diffs with this change, can you take a look at whether they are expected? |
Thanks for running the bot. Diffs are to be expected, some arguments which used to be passed by reference are now enregistered but large amount of performance regressions are worrying. I'll look at it in spare time between RISC-V PRs. |
Note that they are size diffs only, and overall the size improves. I spot checked a few of the regressions and it's usually the result of some new register shuffling because of an empty struct now taking up a register, so I guess those diffs are overall expected. Some of these cases look like they could be switched to static abstracts to avoid the parameter entirely. @jkotas I assume we are ok to take an ABI change like this one since the R2R format is already being bumped? |
Yes. I do not see a problem with the ABI change here. |
The current implementation barred a struct containing empty struct fields from enregistration. This did not match the behavior of GCC & Clang on Linux and the System V ABI which says:
Stems from #101796, part of #84834, cc @dotnet/samsung