Skip to content

Adding Int128 and UInt128 with a base software implementation #69204

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.

Sign up for GitHub

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

Merged
tannergooding merged 20 commits into from
May 19, 2022
Merged

Adding Int128 and UInt128 with a base software implementation #69204

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
897fdb9
Adding barebones Int128 and UInt128 structs
tannergooding Apr 11, 2022
2fe1618
Special case Int128 and UInt128 alignment on x64 Unix and Arm64
tannergooding May 2, 2022
6559609
Implementing Int128 and UInt128
tannergooding May 3, 2022
2c820b5
Adding tests for Int128 and UInt128
tannergooding May 7, 2022
e7970fb
Updating Int128/UInt128 to respect the System V ABI ordering
tannergooding May 11, 2022
828440f
Merge remote-tracking branch 'dotnet/main' into generic-math-int128
tannergooding May 11, 2022
6904e73
Fixing an issue with UInt128->BigInteger setting the wrong sign
tannergooding May 12, 2022
bcbc375
Merge remote-tracking branch 'dotnet/main' into generic-math-int128
tannergooding May 12, 2022
09e8bfc
Don't use Unsafe.As in the Int128/UInt128 hex parsing logic
tannergooding May 12, 2022
5f9d22f
Adding Int128 P/Invoke tests and ensure R2R correctly sets the packing
tannergooding May 13, 2022
b6b85e6
Fixing some issues with the Int128 interop test for non-Windows
tannergooding May 13, 2022
9de4e76
Ensure that floating-point conversions exist for Int128 and UInt128
tannergooding May 13, 2022
a1dc14f
Fixing the casing of a couple fields
tannergooding May 16, 2022
7666952
Revert "Don't use Unsafe.As in the Int128/UInt128 hex parsing logic"
tannergooding May 16, 2022
f0a30cb
Adjusting the Int128/UInt128 generic math tests to have consistent or…
tannergooding May 16, 2022
cb5a82e
Responding to PR feedback
tannergooding May 18, 2022
384e572
Ensure that pNativeLayoutInfo alignment is initialized for Int128/UIn…
tannergooding May 18, 2022
ab85c7b
Don't use Unsafe.As in the Int128/UInt128 hex parsing logic
tannergooding May 12, 2022
163cfda
Merge remote-tracking branch 'dotnet/main' into generic-math-int128
tannergooding May 19, 2022
cd3c9e9
Skip the Interop/PInvoke/Int128 tests on Mono
tannergooding May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Special case Int128 and UInt128 alignment on x64 Unix and Arm64
  • Loading branch information
@tannergooding
tannergooding committed May 11, 2022
commit 2fe1618d2b0926da040ea03893bed55f1580c09a
6 changes: 6 additions & 0 deletions src/coreclr/vm/classnames.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@
#define g_DateTimeOffsetClassName "System.DateTimeOffset"
#define g_DecimalClassName "System.Decimal"

#define g_Int128ClassName "System.Int128"
#define g_Int128Name "Int128"

#define g_UInt128ClassName "System.UInt128"
#define g_UInt128Name "UInt128"

#define g_Vector64ClassName "System.Runtime.Intrinsics.Vector64`1"
#define g_Vector64Name "Vector64`1"

Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/vm/corelib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,9 @@ DEFINE_FIELD(DELEGATE, METHOD_PTR_AUX, _methodPtrAux)
DEFINE_METHOD(DELEGATE, CONSTRUCT_DELEGATE, DelegateConstruct, IM_Obj_IntPtr_RetVoid)
DEFINE_METHOD(DELEGATE, GET_INVOKE_METHOD, GetInvokeMethod, IM_RetIntPtr)

DEFINE_CLASS(INT128, System, Int128)
Copy link
Member

@jkotas jkotas May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere?

Copy link
Member Author

@tannergooding tannergooding May 18, 2022

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 in methodTableBuilder.cpp (https://github.com/dotnet/runtime/pull/69204/files/cb5a82e5df48bf4a7c6deea85765c75d3d4a2e03#diff-4b65f34bf68fdb80cc734cc2435c72ef7f86ef16b9f56af47c06b87761383d3aR9909)

Copy link
Member

@jkotas jkotas May 18, 2022

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

g_Int128Name is defined in classnames.h. This macro defines CLASS_INT128 that I do not see used anywhere.

Copy link
Member Author

@tannergooding tannergooding May 18, 2022

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 the pNativeLayoutInfo: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/classlayoutinfo.cpp#L947-L964

Copy link
Member Author

@tannergooding tannergooding May 18, 2022

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 the Vector* types was blocking interop which isn't required for Int128/UInt128 given the interop tests are all passing.

DEFINE_CLASS(UINT128, System, UInt128)

DEFINE_CLASS(DYNAMICMETHOD, ReflectionEmit, DynamicMethod)

DEFINE_CLASS(DYNAMICRESOLVER, ReflectionEmit, DynamicResolver)
Expand Down
18 changes: 16 additions & 2 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9844,7 +9844,7 @@ void MethodTableBuilder::CheckForSystemTypes()

if (strcmp(nameSpace, g_IntrinsicsNS) == 0)
{
EEClassLayoutInfo * pLayout = pClass->GetLayoutInfo();
EEClassLayoutInfo* pLayout = pClass->GetLayoutInfo();

// The SIMD Hardware Intrinsic types correspond to fundamental data types in the underlying ABIs:
// * Vector64<T>: __m64
Expand All @@ -9854,7 +9854,6 @@ void MethodTableBuilder::CheckForSystemTypes()
// These __m128 and __m256 types, among other requirements, are special in that they must always
// be aligned properly.


if (strcmp(name, g_Vector64Name) == 0)
{
// The System V ABI for i386 defaults to 8-byte alignment for __m64, except for parameter passing,
Expand Down Expand Up @@ -9898,6 +9897,21 @@ void MethodTableBuilder::CheckForSystemTypes()

return;
}
#if defined(UNIX_AMD64_ABI) || defined(TARGET_ARM64)
else if (strcmp(nameSpace, g_SystemNS) == 0)
{
EEClassLayoutInfo* pLayout = pClass->GetLayoutInfo();

// These types correspond to fundamental data types in the underlying ABIs:
// * Int128: __int128
// * UInt128: unsigned __int128

if ((strcmp(name, g_Int128Name) == 0) || (strcmp(name, g_UInt128Name) == 0))
{
pLayout->m_ManagedLargestAlignmentRequirementOfAllMembers = 16; // sizeof(__int128)
Copy link
Member

@jkotas jkotas May 11, 2022

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

Copy link
Member Author

@tannergooding tannergooding May 13, 2022

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 and UInt128 need to have 16-byte packing when targeting the System V ABI (used by Unix) and the same packing/alignment as Int64/UInt64 otherwise (such as on Windows or 32-bit platforms).

Copy link
Member Author

@tannergooding tannergooding May 13, 2022

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.

Copy link
Contributor

@vargaz vargaz May 13, 2022

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.

Copy link
Member Author

@tannergooding tannergooding May 13, 2022

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).

Copy link
Member Author

@tannergooding tannergooding May 13, 2022

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 existing Vector64<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.

Copy link
Contributor

@vargaz vargaz May 14, 2022

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.

Copy link
Member Author

@tannergooding tannergooding May 14, 2022

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).

Copy link
Member Author

@tannergooding tannergooding May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged #69399

}
}
#endif // UNIX_AMD64_ABI || TARGET_ARM64
}

if (g_pNullableClass != NULL)
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Int128.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System
{
/// <summary>Represents a 128-bit signed integer.</summary>
[Intrinsic]
[StructLayout(LayoutKind.Sequential)]
public readonly struct Int128
{
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/UInt128.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System
{
/// <summary>Represents a 128-bit unsigned integer.</summary>
[Intrinsic]
[StructLayout(LayoutKind.Sequential)]
public readonly struct UInt128
{
Expand Down