-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RISC-V][x64] WiP: Passing empty struct fields #101796
Closed
Closed
Changes from 58 commits
Commits
Show all changes
70 commits
Select commit
Hold shift + click to select a range
c2c2bba
First, add a bunch of failing tests
tomeksowi 03d4b23
Reduce empty megabyte field to 32k as msvc caps size of arguments at 64k
tomeksowi 4403c4b
Don't stop calculating flags if struct size > 16 bytes
tomeksowi bf81116
Classify empty structs on x64 like padding
tomeksowi 60ad834
Quell C-linkage warnings just in case
tomeksowi 9064784
Add Pack=1 to bypass a known problem with field alignment requirement…
tomeksowi 002c4b2
Merge branch 'main' into empty-struct-passing
tomeksowi b02c913
Check `size > 16` only when passing parameters according to integer c…
tomeksowi 0fde22d
Don't assume struct size > 16 means return by implicit ref to a retur…
tomeksowi f89a68d
Don't assume if struct size > 16, IsArgPassedByRef should return true…
tomeksowi f5e3930
Adjust ArgIterator.GetNextOffset() for crossgen2 to be the same as th…
tomeksowi 0a5ce05
Adjust crossgen2 version of ComputeReturnFlags (ComputeReturnValueTre…
tomeksowi 6a3118b
Adjust IsArgPassedByRef for crossgen2 with native version for VM. Als…
tomeksowi 0a1a7ff
Don't assume struct (size > 16) means pass by reference in Compiler::…
tomeksowi 9aa2d10
Add a test for a single-float struct padded with empty struct field
tomeksowi 9d9d0e2
Relax checks so the new test cases JIT without false positive assertions
tomeksowi 983d423
New structure to store information about passing structs according to…
tomeksowi 8366470
Implement GetRiscV64PassFPStructInfo for now with checks against exis…
tomeksowi c3e8578
Rename to match similar functions on other architectures
tomeksowi c2dd449
Use bitfields to keep the most important flags for decision making pa…
tomeksowi 0d81d68
Fix JIT for structs with single float padded with empty structs
tomeksowi ec8d404
Replace bitfields with manual flags to avoid potential portability pr…
tomeksowi 98e1861
Update C# version of GetRiscV64PassFpStructInRegistersInfo
tomeksowi 623a2a6
Replace getRISCV64PassStructInRegisterFlags with getRiscV64PassFpStru…
tomeksowi f550c22
Fix offset assignment in C# GetRiscV64PassFpStructInRegistersInfo
tomeksowi 338e244
Use enregistered struct field offsets in JIT new ABI classifiers
tomeksowi 894fccd
Merge branch 'main' into empty-struct-passing
tomeksowi 4cdd228
Fix build: formatting and using static ordering
tomeksowi 99ec678
Include GC info in FpStructInRegistersInfo like on System V. While it…
tomeksowi b5e1cdc
Nicer size shift calculation routine
tomeksowi 95e787d
Fix build
tomeksowi 48e7944
Fix Empty8Float test
tomeksowi 7102f31
Add EmptyFloatEmpty5(U)Byte tests
tomeksowi e5d67cd
Increase field visibility to match other tests
tomeksowi e25552e
Fix EmptyFloatEmpty5(U)Byte and LongEmptyDouble tests by using correc…
tomeksowi 28a88b3
Fix LongEmptyGDoubleByImplicitRef and the rest of tests in StructABI …
tomeksowi 123eaaf
Make tests harder
tomeksowi 1a85e0b
Improve logging for GetRiscV64PassFpStructInRegistersInfo, similar to…
tomeksowi 6163536
Format fix
tomeksowi 6dc499d
Fix test EchoArrayOfEmptiesFloatDouble by returning FpStruct{ UseIntC…
tomeksowi afd95cd
Fix false positive marking a struct split between register and stack
tomeksowi de83bc7
Add more tests for cases where structs eligible for hardware floating…
tomeksowi 978e851
Fix test EchoEmptyFloatEmpty5ByteSplitRiscV
tomeksowi dab9167
Remove LclVarDsc::lvIs4Field{1,2} because they were write-only
tomeksowi 2d24bee
Fix ArgIteratorTemplate::GetNextOffset() for struct{Arr[], float}
tomeksowi 9eb6b00
Merge branch 'main' into getnextoffset-for-struct-ptr-float
tomeksowi d3e974c
Merge branch 'getnextoffset-for-struct-ptr-float' into empty-struct-p…
tomeksowi a0ccb2a
Merge branch 'main' into empty-struct-passing
tomeksowi f469ef5
Fix arm32 build by reverting to using cSlotsToEnregister in lclvars.
tomeksowi 811f947
Add failing tests for hardware floating-point calling convention by r…
tomeksowi b6bef68
Merge branch 'main' into empty-struct-passing
tomeksowi ac2670b
Fix small struct passing to calls by reflection: CopyStructToRegister…
tomeksowi 76533b6
Fix FP structs returned from calls by reflection
tomeksowi c7e8a11
Fix the rest of the reflection tests by properly determining whether …
tomeksowi 16b549f
Update crossgen2 C# version of ArgIterator to changes on the native side
tomeksowi 3389bcf
Fix copying structs returned by hardware floating-point calling conve…
tomeksowi d4f81be
Merge branch 'main' into empty-struct-passing
tomeksowi 7551d35
Merge branch 'main' into empty-struct-passing
tomeksowi 4d2e2cb
Add signedness to integer field FpStructPassInRegistersInfo
tomeksowi 907ac95
Better flag names
tomeksowi 892ec22
Improve explanation why RISC-V can't use genActualType in genParamSta…
tomeksowi e73450e
Take signedness into account in CopyStructToRegisters
tomeksowi 717cd59
Remove IsSize(1st|2nd)8 because they weren't used much
tomeksowi 64de193
Improve getter names in FpStructInRegistersInfo
tomeksowi d2d1841
Add comment to fix JIT to AssignClassifiedEightByteTypes
tomeksowi 47f9d87
Add helpers for IntKind and flag names to FpStructInRegistersInfo
tomeksowi 09c0b38
Merge branch 'main' into empty-struct-passing
tomeksowi 8d6a102
Fix C# build: Enum values should be on separate lines
tomeksowi 5ebd2af
Merge branch 'main' into empty-struct-passing
tomeksowi 7036dc0
Make a logging wrapper Compiler::GetPassFpStructInRegistersInfo, simi…
tomeksowi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share some information about the case this fixes? Given that promoted struct fields are normalize-on-load, this special case should not be necessary unless there is a bug elsewhere in the RISCV64/LA64 backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was for cases like this:
runtime/src/tests/JIT/Directed/StructABI/StructABI.cs
Lines 691 to 707 in 7551d35
Homing the argument in the EmptyFloatEmpty5Byte:Equals prolog trashed the this pointer:
I saw native compilers home arguments with appropriately-sized stores at original struct offsets so I fixed it the same way.
Full JitDump of EmptyFloatEmpty5Byte:Equals if interested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I think the fix makes sense. The comment doesn't seem fully accurate then; the real problem seems to be that since the enregistered layout does not match the memory layout of the struct, rounding up the size would potentially extend outside the stack slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'll reword the comment.
Oh, and please view this PR as a proof of concept. I need to upstream it in smaller chunks so it can be reviewed with some confidence.