Adding Int128 and UInt128 with a base software implementation by tannergooding · Pull Request #69204 · dotnet/runtime

tannergooding · 2022-05-11T19:56:08Z

This does not cover acceleration in the JIT, that will be a separate work item and will be handled in a follow up PR.

ghost · 2022-05-11T19:56:13Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-05-11T19:56:15Z

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

This does not cover acceleration in the JIT, that will be a separate work item and will be handled in a follow up PR.

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Numerics`, `new-api-needs-documentation`
Milestone:	-

jkotas · 2022-05-11T20:52:31Z

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


This needs equivalent fix in crossgen/NativeAOT and in Mono

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

-- I fixed this up for crossgen/NativeAOT already and added corresponding P/Invoke tests to ensure the data is passed correctly to/from Native.

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.

This doesn't impact 32-bit platforms, only 64-bit platforms (and only 64-bit Unix at that).

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

Yes, it looks like this is not implemented right now in mono.

I'll log an issue to ensure this is tracked (I don't see an existing issue).

Logged #69399

tannergooding · 2022-05-12T03:07:00Z

Hmmm. Tests were all passing locally but failing in CI, going to need to investigate this more

tannergooding · 2022-05-13T00:02:02Z

This should just need the NativeAOT and Mono fixups to ensure Int128 and UInt128 are Pack = 16 on Sys V (x64 and Arm64)

tannergooding · 2022-05-13T00:10:23Z

I also apparently forgot to implement conversions to and from float/double/decimal, so I will do that and add tests. Marking as NO-MERGE in the meantime

deeprobin

Have noted a few minor things.
But looks very good.

Looking forward to Int128 and UInt128 🎉😄.

azure-pipelines · 2022-05-18T15:03:40Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2022-05-18T15:07:19Z

ibraries Test Run checked coreclr Linux arm Debug
ibraries Test Run checked coreclr Linux_musl arm64 Debug

Both these runs failed with GC asserts. I do not think we have any issues tracking failures like this. Is it caused by the changes in this PR?

tannergooding · 2022-05-18T15:33:46Z

Both these runs failed with GC asserts. I do not think we have any issues tracking failures like this. Is it caused by the changes in this PR?

I'll take a look but I find it doubtful that it'd be caused by this PR. It wasn't failing before and the only change since the last run was to update the native layout for Int128/UInt128. Neither of these types are used by the Regex or Http tests.

Http is failing BOOL MethodTable::SanityCheck() for:

 // strings have component size2, all other non-arrays should have 0
_ASSERTE((GetComponentSize() <= 2) || IsArray());

The other (Regex) is failing _ASSERTE(RawGetMethodTable()) in void CObjectHeader::SetMarked()

dakersnar

Changes look good, thanks!

jkotas · 2022-05-18T16:05:15Z

Neither of these types are used by the Regex or Http tests.

These asserts are generic signs of GC heap corruption. A bug introduced in common parts (e.g. parsing/formatting) can lead to intermittent crash like that.

tannergooding · 2022-05-18T16:19:36Z

These asserts are generic signs of GC heap corruption. A bug introduced in common parts (e.g. parsing/formatting) can lead to intermittent crash like that.

I'd speculate there may be another issue with Unsafe.As then and there is still something wrong with 7666952

There was a similar issue exposed by making Unsafe.As "intrinsic" that was fixed in #69271 and its possible this is another case that hasn't been found yet.

tannergooding · 2022-05-18T16:24:30Z

It could be related to #69501 for example, which @SingleAccretion just opened and impacts structs.

tannergooding · 2022-05-18T16:29:59Z

If this one is also related to Unsafe.As, then it may be worth reverting just that one from intrinsic handling for the time being, as this will have been the 3rd (iirc) GC related issue it exposed.

tannergooding · 2022-05-18T16:42:31Z

I'm not 100% this fixes the issue as its not a reliable repro. But I did hit it in 20 runs with the Unsafe.As usage and didn't hit it in 40 without it removed.

jkotas · 2022-05-18T16:43:53Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-18T16:44:11Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2022-05-18T17:19:51Z

I'm not 100% this fixes the issue as its not a reliable repro. But I did hit it in 20 runs with the Unsafe.As usage and didn't hit it in 40 without it removed.

Can you give instructions for how to repro this? If we have a repro for it we need to look at it, it could be manifesting in other places that are way harder to diagnose.

tannergooding · 2022-05-18T17:36:15Z

Can you give instructions for how to repro this? If we have a repro for it we need to look at it, it could be manifesting in other places that are way harder to diagnose.

It was just running the Int128/UInt128 tests with and without the Unsafe.As. I imagine it would reproduce for any non-primitive struct.

jkotas · 2022-05-18T21:00:38Z

The new Int128 PInvoke test is failing on Android in the extra platforms leg.

tannergooding · 2022-05-18T21:06:45Z

Looks like it can't resolve the native library: System.DllNotFoundException: Int128Native

@vargaz is there anything additional required for P/Invoke to work on Android? Does the library have or need some special name that differs from the default for example?

tannergooding · 2022-05-18T21:16:54Z

The tvOS and iOS failures look unrelated, they seem to have just failed and look to be doing that in other jobs as well.

tannergooding · 2022-05-18T21:30:21Z

Hmmm, it looks like most of the Interop/PInvoke/* tests are currently disabled on Mono. See https://github.com/dotnet/runtime/blob/main/src/tests/issues.targets#L1716-L1772 under https://github.com/dotnet/runtime/blob/main/src/tests/issues.targets#L1446-L1447; and more further down and in other mono areas.

Most are marked "needs triage" as well.

tannergooding · 2022-05-19T00:10:33Z

There are various other extra-platforms failures but the android issue is the only one that looks related. Others look to be failures more broadly impacting these jobs in particular, many of them are just general timeouts at 240 minutes.

Given the breadth of P/Invoke tests disabled for Android, I'd be inclined to log an issue tracking Mono enabling interop here. The alternative would be to just block Int128/UInt128 from all interop until Mono can add support. The tests likely need to be disabled on mono regardless given that the native library isn't resolving.

jkotas · 2022-05-19T00:39:40Z

Disabling the interop test on Android should be fine.

tannergooding · 2022-05-19T00:51:00Z

Logged #69531.

Noting I grouped this with the other Mono P/Invoke disablement as tvOS and iOS didn't actually run the tests, they hit the more general timeout issue.

tannergooding added 5 commits May 10, 2022 20:01

Adding barebones Int128 and UInt128 structs
897fdb9

Special case Int128 and UInt128 alignment on x64 Unix and Arm64
2fe1618

Implementing Int128 and UInt128
6559609

Adding tests for Int128 and UInt128
2c820b5

Updating Int128/UInt128 to respect the System V ABI ordering
e7970fb

ghost assigned tannergooding May 11, 2022

ghost added area-System.Numerics new-api-needs-documentation labels May 11, 2022

Perksey reviewed May 11, 2022
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/UInt128.cs Outdated

jkotas reviewed May 11, 2022
View reviewed changes

tannergooding added 2 commits May 11, 2022 14:02

Merge remote-tracking branch 'dotnet/main' into generic-math-int128
828440f

Fixing an issue with UInt128->BigInteger setting the wrong sign
6904e73

Merge remote-tracking branch 'dotnet/main' into generic-math-int128
bcbc375

runfoappBot mentioned this pull request May 12, 2022
jit.hardwareintrinsics.general.notsupported failing recently #69267
Closed

Don't use Unsafe.As in the Int128/UInt128 hex parsing logic
09e8bfc

This was referenced May 12, 2022
jit.1 work item failing on mono #67888
Closed
Test failure JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-5/regex-redux-5.sh #66625
Closed

Adding Int128 P/Invoke tests and ensure R2R correctly sets the packing
5f9d22f

tannergooding requested a review from MichalStrehovsky as a code owner May 13, 2022 04:08

tannergooding added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 13, 2022

tannergooding added 2 commits May 13, 2022 06:10

Fixing some issues with the Int128 interop test for non-Windows
b6b85e6

Ensure that floating-point conversions exist for Int128 and UInt128
9de4e76

tannergooding removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 13, 2022

deeprobin approved these changes May 15, 2022
View reviewed changes

Comment threadsrc/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Int128.cs
Comment threadTHIRD-PARTY-NOTICES.TXT

Fixing the casing of a couple fields
a1dc14f

dakersnar approved these changes May 18, 2022
View reviewed changes

Don't use Unsafe.As in the Int128/UInt128 hex parsing logic
ab85c7b

tannergooding added 2 commits May 18, 2022 17:45

Merge remote-tracking branch 'dotnet/main' into generic-math-int128
163cfda

Skip the Interop/PInvoke/Int128 tests on Mono
cd3c9e9

tannergooding mentioned this pull request May 19, 2022
Enable Int128/UInt128 interop tests for Mono #69531
Open

tannergooding merged commit 7f9ed8f into dotnet:mainMay 19, 2022

tannergooding mentioned this pull request May 19, 2022
Add support for Int128 and UInt128 data types #67151
Closed

AntonLapounov mentioned this pull request May 24, 2022
Failures on Mono Android x64 Release @ Ubuntu.1804.Amd64.Android.29.Open on rolling build of runtime-extra-platforms #69678
Closed

mkhamoyan mentioned this pull request May 24, 2022
Disable test with the correct path #69693
Merged

ghost locked as resolved and limited conversation to collaborators Jun 18, 2022

tannergooding deleted the generic-math-int128 branch November 11, 2022 15:36

Uh oh!

Conversation

tannergooding commented May 11, 2022

Uh oh!

ghost commented May 11, 2022

Uh oh!

ghost commented May 11, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding commented May 12, 2022

Uh oh!

tannergooding commented May 13, 2022

Uh oh!

tannergooding commented May 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deeprobin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

azure-pipelinesBot commented May 18, 2022

Uh oh!

jkotas commented May 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

dakersnar left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented May 18, 2022

Uh oh!

tannergooding commented May 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

jkotas commented May 18, 2022

Uh oh!

azure-pipelinesBot commented May 18, 2022

Uh oh!

jakobbotsch commented May 18, 2022

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

jkotas commented May 18, 2022

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

tannergooding commented May 18, 2022

Uh oh!

tannergooding commented May 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented May 13, 2022•
edited
Loading

jkotas commented May 18, 2022•
edited
Loading

tannergooding commented May 18, 2022•
edited
Loading

tannergooding commented May 19, 2022•
edited
Loading

jkotas commented May 19, 2022•
edited
Loading

tannergooding commented May 19, 2022•
edited
Loading