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.
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
Adding Int128 and UInt128 with a base software implementation #69204
Adding Int128 and UInt128 with a base software implementation #69204
Changes from all commits
897fdb9
2fe1618
6559609
2c820b5
e7970fb
828440f
6904e73
bcbc375
09e8bfc
5f9d22f
b6b85e6
9de4e76
a1dc14f
7666952
f0a30cb
cb5a82e
384e572
ab85c7b
163cfda
cd3c9e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this used anywhere?
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.
This is what defines
g_Int128
which is used inmethodTableBuilder.cpp
(https://github.com/dotnet/runtime/pull/69204/files/cb5a82e5df48bf4a7c6deea85765c75d3d4a2e03#diff-4b65f34bf68fdb80cc734cc2435c72ef7f86ef16b9f56af47c06b87761383d3aR9909)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.
g_Int128Name
is defined in classnames.h. This macro definesCLASS_INT128
that I do not see used anywhere.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.
Ah right, I was getting two two things mixed up.
Looks like this is missing handling in
classlayoutinfo.cpp
which copies the alignment requirement to thepNativeLayoutInfo
: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/classlayoutinfo.cpp#L947-L964There 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.
Should be fixed. The only other usage of
CLASS__*
for theVector*
types was blocking interop which isn't required for Int128/UInt128 given the interop tests are all passing.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.
This needs equivalent fix in crossgen/NativeAOT and in Mono
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.
@fanyang-mono could you point me to where
Mono
handles custom struct packing/alignment?I tried checking for where that's handled for the vector types, but couldn't find it.
Int128
andUInt128
need to have 16-byte packing when targeting theSystem V
ABI (used by Unix) and the same packing/alignment asInt64
/UInt64
otherwise (such as on Windows or 32-bit platforms).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.
-- I fixed this up for crossgen/NativeAOT already and added corresponding P/Invoke tests to ensure the data is passed correctly to/from Native.
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's handled in mono_class_layout_fields () in metadata/class-init.c.
Not sure the mono gc supports 16 byte aligned objects on 32 bit platforms.
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.
This doesn't impact 32-bit platforms, only 64-bit platforms (and only 64-bit Unix at that).
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.
@vargaz, looks like its just blocked everywhere not just 32-bit platforms: https://github.com/dotnet/runtime/blob/main/src/mono/mono/metadata/class-init.c#L2058-L2065
It looks like Mono also isn't differentiating "alignment" from "packing" and so the layout of types that include the new
Int128
,UInt128
, or the existingVector64<T>
,Vector128<T>
,Vector256<T>
types will be incorrect.This would explain why I couldn't find any logic touching
Vector128<T>
and ensuring that it has the right packing (and therefore layout) even if the GC couldn't correctly align the overall allocation.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.
Yes, it looks like this is not implemented right now in mono.
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.
I'll log an issue to ensure this is tracked (I don't see an existing issue).
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.
Logged #69399