Alternate approach for HELPER_METHOD_FRAME removal for arithmetic div… #113286

davidwrighton · 2025-03-08T00:13:50Z

This is a combination of @am11's work in PR #109087 and some work to just rewrite the Windows X86 helpers in assembly.

For non 64-bit platforms, remove the helpers
For Unix platforms, rely on an implementation which uses an FCall to native code to invoke the various operations.
For Windows X86 on CoreCLR, rewrite the helpers in assembly with a tail-call approach for throwing.
- Also it was noted that the existing helpers do a fair bit of unnecessary stack manipulation, and the manual rewrite avoids all of that. Maybe this will actually be faster. (Turns out it is about the same on performance. The real win would come from re-ordering the argument order, but that's a much more invasive change in the JIT, which I judge to be too risky)

…ide operations - Instead of moving to managed - For non 64-bit platforms, remove the helpers - For Unix platforms, rely on [[clang::musttail]] to tail-call a helper which throws (current implementation still uses FCThrow, but that should be relatively easy to move to a call to the managed throw routines - For Windows X86, rewrite the helpers in assembly with a tail-call approach for throwing. - As part of writing the helpers in assembly, it was noted that the stack layout is swapped from the underyling CRT routines for 64bit arithmetic. Tweak the helper calls for those cases to pass in data in the order which the CRT is going to handle them efficiently. Do this for all architectures/OSs as other ones don't really care about this weirdness. - Also it was noted that the existing helpers do a fair bit of unnecessary stack manipulation, and the manual rewrite avoids all of that. Maybe this will actually be faster. TODO: Bump jit version TODO: Bump R2R version TODO: Actually use managed helper to throw instead of keeping the FCThrow in place

jkoritzinsky · 2025-03-08T01:17:23Z

src/coreclr/vm/jithelpers.cpp

+// Using the musttail attribute requires that we disble our protection against using return in the wrong place
+#ifdef return
+#undef return
+#endif


Related: I'm trying to remove the cases outside of HMFs that engage this infra as it continues to cause us problems in places.

am11 · 2025-03-08T01:39:43Z

src/coreclr/vm/jithelpers.cpp

+    {
+        if (divisor == 0)
+        {
+            [[clang::musttail]] return JIT_ThrowDivideByZero_RetInt();


We can define MUSTTAIL for this:

#if TARGET_UNIX #ifdef __clang__ #define MUSTTAIL [[clang::musttail]] #else // __clang__ #define MUSTTAIL [[gnu::musttail]] #endif // __clang__ #endif // TARGET_UNIX

am11 · 2025-03-08T01:41:12Z

src/coreclr/vm/jithelpers.cpp

+{
+    FCALL_CONTRACT;
+
+    FCThrow(kOverflowException);


Could we directly inline them in JIT? there already are CORINFO_HELP_OVERFLOW and CORINFO_HELP_THROWDIVZERO exposed.

Yes. This is just an experiment to see how musttail works, and if it can be used to help solve the HMF problem without changing the JIT/etc for this issue. Given the number of test failures, something clearly didn't go completely right with my change here, so I want to understand what I've missed.

…n arm32 build environment...

jkotas · 2025-03-08T13:26:29Z

src/coreclr/inc/jithelpers.h

+    // CORINFO_HELP_DBL2INT, CORINFO_HELP_DBL2UINT, and CORINFO_HELP_DBL2LONG get
+    // patched for CPUs that support SSE2 (P4 and above).


Suggested change

// CORINFO_HELP_DBL2INT, CORINFO_HELP_DBL2UINT, and CORINFO_HELP_DBL2LONG get

// patched for CPUs that support SSE2 (P4 and above).

Outdated comment

…the original order (Sacrifices perf, but avoid needinga bunch of optimizer changes) Adjust the negate path for the X86 hand coded assembly for JIT_LDiv to load edx before jumping to JIT_LDiv_DoNegate

… the FCThrow machinery

…etic

…hat the musttail feature doesn't work on Linux X86 very well, but it DOES seem to work on arm, so there is at least 1 platform where it works

…etic

am11 · 2025-03-21T21:15:10Z

src/coreclr/vm/jithelpers.cpp

+
+#if TARGET_UNIX
+#ifdef __clang__
+#define MUSTTAIL [[clang::musttail]]


BTW, the performance concern was only for win-x86 and performance optimizations aren't critical for linux-arm and linux-x86. Moreover, it seems like MUSTTAIL is only to accelerate exception path with no effect on the actual div/mod ops? If this is the case, then I think we can just drop this optimization even if it was for win-x86 because perf concern was about the div/mod (non-exception) happy path.

The MUSTTAIL logic here isn't an optimization as without HMFs we can't through an exception from an FCALL, but I agree, it doesn't look like it has worked out as a reasonable way to make this work simple. My current plan is to keep the work I've done to port the Windows X86 helpers to handcoded assembly, and merge it with @am11's work that is more general purpose, and then call it good.

…te_approach_to_removingHMF_for_arithmetic

am11 · 2025-03-24T22:02:10Z

There was also a pure managed experiment somewhere in the middle using generic math with no FCall that was passing all tests:

runtime/src/libraries/System.Private.CoreLib/src/System/Math.DivModInt.cs

Lines 180 to 243 in e51389e

    
           private static (T quotient, T remainder) DivMod<T, U>(T dividend, T divisor) 
        
               where T : INumber<T>, IMinMaxValue<T> 
        
               where U : INumber<U>, IMinMaxValue<U>, IShiftOperators<U, int, U>, IBitwiseOperators<U, U, U> 
        
           { 
        
               bool dividendIsNegative = false; 
        
               bool divisorIsNegative = false; 
        
               // Handle signs if T is signed 
        
               if (typeof(T) != typeof(U)) 
        
               { 
        
                   if (dividend < T.Zero) 
        
                   { 
        
                       dividend = -dividend; 
        
                       dividendIsNegative = true; 
        
                   } 
        
                   if (divisor < T.Zero) 
        
                   { 
        
                       divisor = -divisor; 
        
                       divisorIsNegative = true; 
        
                   } 
        
               } 
        
               // Perform unsigned division using type U 
        
               U uDividend = U.CreateTruncating(dividend); 
        
               U uDivisor = U.CreateTruncating(divisor); 
        
               U bit = U.One; 
        
               U uQuotient = U.Zero; 
        
               int mask = typeof(U) == typeof(uint) ? 31 : 63; 
        
               // Align divisor with dividend 
        
               while (uDivisor < uDividend && bit != U.Zero && U.IsZero(uDivisor & (U.One << mask))) 
        
               { 
        
                   uDivisor <<= 1; 
        
                   bit <<= 1; 
        
               } 
        
               // Perform the division 
        
               while (bit > U.Zero) 
        
               { 
        
                   if (uDividend >= uDivisor) 
        
                   { 
        
                       uDividend -= uDivisor; 
        
                       uQuotient |= bit; 
        
                   } 
        
                   bit >>= 1; 
        
                   uDivisor >>= 1; 
        
               } 
        
               // Convert results back to type T 
        
               T tQuotient = T.CreateTruncating(uQuotient); 
        
               T tRemainder = T.CreateTruncating(uDividend); 
        
               // Adjust signs if T is signed 
        
               if (typeof(T) != typeof(U)) 
        
               { 
        
                   if (dividendIsNegative) 
        
                       tRemainder = -tRemainder; 
        
                   if (dividendIsNegative ^ divisorIsNegative) 
        
                       tQuotient = -tQuotient; 
        
               } 
        
               return (tQuotient, tRemainder); 
        
           }

(those InternalCalls after line 243 are unused)

It was quite slow EgorBot/runtime-utils#193. (was probably dreaming that it could outperform the native implementation 🥲)

The FCall version you've applied was performing much better EgorBot/runtime-utils#191, which is fine for 32-bit linux.

jkotas · 2025-03-24T22:40:08Z

src/libraries/System.Private.CoreLib/src/System/Math.DivModInt.cs

+        [MethodImpl(MethodImplOptions.InternalCall)]
+        private static extern uint DivUInt32Internal(uint dividend, uint divisor);
+
+#if NATIVEAOT


Can this be same between NAOT and regular CoreCLR?

AOT could use InternalCall (with revert of RuntimeHelpers.cs file). There was a slight regression with QCall on coreclr.

Right, my point is that there is no reason for two to be different.

(I do not have a strong opinion on FCall vs. QCall here as long as it is the same.)

…te_approach_to_removingHMF_for_arithmetic

am11

Number are looking promising EgorBot/runtime-utils#336 (comment) (0.55) 🚀

am11 · 2025-04-01T21:42:28Z

src/coreclr/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.

+using System;


…te_approach_to_removingHMF_for_arithmetic

jkotas · 2025-04-04T15:14:47Z

src/coreclr/inc/jithelpers.h

+#else // TARGET_64BIT
+    DYNAMICJITHELPER(CORINFO_HELP_LMUL_OVF,     NULL,               METHOD__NIL)
+    DYNAMICJITHELPER(CORINFO_HELP_ULMUL_OVF,    NULL,               METHOD__NIL)
+    JITHELPER(CORINFO_HELP_LDIV,                NULL,           METHOD__NIL)


Padding for the METHOD__NIL column is off.

jkotas · 2025-04-04T15:17:51Z

src/coreclr/inc/jithelpers.h

+    DYNAMICJITHELPER(CORINFO_HELP_ULMOD,        NULL,               METHOD__MATH__MOD_UINT64)
+#endif // TARGET_X86 && TARGET_WINDOWS
+#else // TARGET_64BIT
+    DYNAMICJITHELPER(CORINFO_HELP_LMUL_OVF,     NULL,               METHOD__NIL)


If CORINFO_HELP_LMUL/ULMUL_OVF are unused on 64-bit, I would expect them to be JITHELPER.

If they are used, I would expect them to look like the 32-bit ones and be outside the #ifndef TARGET_64BIT ifdef.

…ach_to_removingHMF_for_arithmetic

jkotas

Thanks!

am11 · 2025-04-04T17:47:47Z

Thanks for this @davidwrighton!

For the remaining alloc-like HMFs, I think we can either use managed RuntimeTypeHandle_InternalAlloc{NoChecks} or the assembly ones in AllocFast.{asm,S} (+ the FOH):

runtime/src/coreclr/vm/jithelpers.cpp

Line 622 in b3188c5

newobj = TryAllocateFrozenObject(pMT);

cc @AaronRobinsonMSFT

akoeplinger · 2025-04-06T09:13:31Z

@davidwrighton @am11 this causes an issue on the runtime sdk codeflow on win-x86: dotnet/sdk#48034

finalizer.obj : error LNK2001: unresolved external symbol RhpDbl2Int [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol RhpDbl2Lng [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol RhpLng2Dbl [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol RhpULng2Dbl [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol ceil [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol cos [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol floor [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol modf [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol pow [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol sin [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
finalizer.obj : error LNK2001: unresolved external symbol tan [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]
D:\a\_work\1\vmr\src\sdk\artifacts\bin\finalizer\Release\net10.0-windows\native\finalizer.exe : fatal error LNK1120: 11 unresolved externals [D:\a\_work\1\vmr\src\sdk\src\Layout\finalizer\finalizer.csproj]

am11 · 2025-04-06T10:05:19Z

Ah, nvm, it is also failing in AOT outerloop: https://github.com/dotnet/runtime/runs/40052903604. Although we haven't touched these exports. 👀

akoeplinger · 2025-04-06T10:08:33Z

Ok thanks, I'll prepare a revert since this is blocking the codeflow.

ghost added the area-VM-coreclr label Mar 8, 2025

dotnet-policy-service bot assigned davidwrighton Mar 8, 2025

davidwrighton added 2 commits March 7, 2025 16:39

Update alignment to be a bit better for the assembly routines

e2273af

Try to adjust jithelpers.cpp for better building on arm32

29364c3

jkoritzinsky reviewed Mar 8, 2025

View reviewed changes

am11 reviewed Mar 8, 2025

View reviewed changes

davidwrighton added 3 commits March 7, 2025 18:54

Fix more oops in the build

f7f6e3d

More attempts to fix build breaks... maybe I should actually set up a…

52e28a9

…n arm32 build environment...

Get the types right for JIT_UDiv and JIT_UMod

6796ab4

build-analysis bot mentioned this pull request Mar 8, 2025

Android emulator not booting completely on Helix queue dotnet/dnceng#1448

Open

3 tasks

jkotas reviewed Mar 8, 2025

View reviewed changes

Swap the args around for the 64bit long/ulong divide helpers back to …

7ed11fe

…the original order (Sacrifices perf, but avoid needinga bunch of optimizer changes) Adjust the negate path for the X86 hand coded assembly for JIT_LDiv to load edx before jumping to JIT_LDiv_DoNegate

davidwrighton mentioned this pull request Mar 18, 2025

Convert all FCalls with HELPER_METHOD_FRAME to QCalls #95695

Closed

25 tasks

davidwrighton added 5 commits March 17, 2025 17:14

Attempt to fix 32bit linux builds, and remove all use of FCThrow, and…

e76b3aa

… the FCThrow machinery

Fix BCL build

a5af1b4

Merge branch 'main' into alternate_approach_to_removingHMF_for_arithm…

19ec62a

…etic

Get this approach building on Linux X86 and Linux Arm ... turns out t…

3799b0d

…hat the musttail feature doesn't work on Linux X86 very well, but it DOES seem to work on arm, so there is at least 1 platform where it works

Merge branch 'main' into alternate_approach_to_removingHMF_for_arithm…

f418940

…etic

build-analysis bot mentioned this pull request Mar 21, 2025

BinderTracingTest.ResolutionFlow times out #104670

Open

am11 reviewed Mar 21, 2025

View reviewed changes

davidwrighton added 2 commits March 24, 2025 14:19

Integrate am11's work into my branch

8d6ba7e

Merge branch 'main' of https://github.com/dotnet/runtime into alterna…

ee49f1c

…te_approach_to_removingHMF_for_arithmetic

jkotas reviewed Mar 24, 2025

View reviewed changes

build-analysis bot mentioned this pull request Mar 25, 2025

[wasm][AOT] emcc : error - received SIGKILL (-9) #89402

Open

davidwrighton added 3 commits March 31, 2025 11:19

Fix x86 eh stuff

a87b046

Merge branch 'main' of https://github.com/dotnet/runtime into alterna…

8cc6d52

…te_approach_to_removingHMF_for_arithmetic

Move the math helpers to be consistently FCalls

37ae63f

am11 approved these changes Apr 1, 2025

View reviewed changes

am11 reviewed Apr 1, 2025

View reviewed changes

davidwrighton added 2 commits April 2, 2025 15:09

Remove unused System namespace import

eda6d06

Merge branch 'main' of https://github.com/dotnet/runtime into alterna…

0eb3870

…te_approach_to_removingHMF_for_arithmetic

davidwrighton force-pushed the alternate_approach_to_removingHMF_for_arithmetic branch from a17d34e to 0eb3870 Compare April 2, 2025 22:23

This was referenced Apr 3, 2025

System.Net.WebSockets.Client.Tests timeout #114153

Closed

System.Net.Security.Unit.Tests timeout #114176

Closed

davidwrighton mentioned this pull request Apr 3, 2025

osx-x64 Release AllSubsets_Mono_Minijit_RuntimeTests minijit build leg fails to build with an error referencing a macosx12.0 #114248

Closed

davidwrighton marked this pull request as ready for review April 3, 2025 23:58

davidwrighton requested a review from MichalStrehovsky as a code owner April 3, 2025 23:58

This was referenced Apr 4, 2025

System.Net.Security.Tests timeout #114152

Closed

CI flakiness: Mac builds failing with SDK errors #114235

Closed

am11 requested a review from jkotas April 4, 2025 14:24

jkotas reviewed Apr 4, 2025

View reviewed changes

davidwrighton added 3 commits April 4, 2025 09:23

AAGGHH

a21f51d

Merge branch 'main' of github.com:dotnet/runtime into alternate_appro…

5b7bfb6

…ach_to_removingHMF_for_arithmetic

Code review comments

b3188c5

jkotas approved these changes Apr 4, 2025

View reviewed changes

am11 mentioned this pull request Apr 4, 2025

Move arithmetic DivideByZero and Overflow exceptions to JIT on 32-bit platforms #110226

Closed

davidwrighton merged commit 7aa0868 into dotnet:main Apr 4, 2025
154 of 159 checks passed

akoeplinger mentioned this pull request Apr 6, 2025

Revert "Alternate approach for HELPER_METHOD_FRAME removal for arithmetic div…" #114308

Merged

github-actions bot locked and limited conversation to collaborators May 7, 2025

		// CORINFO_HELP_DBL2INT, CORINFO_HELP_DBL2UINT, and CORINFO_HELP_DBL2LONG get
		// patched for CPUs that support SSE2 (P4 and above).

Alternate approach for HELPER_METHOD_FRAME removal for arithmetic div… #113286

Alternate approach for HELPER_METHOD_FRAME removal for arithmetic div… #113286

Uh oh!

Conversation

davidwrighton commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

am11 commented Mar 24, 2025

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!

am11 left a comment

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!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

am11 commented Apr 4, 2025

Uh oh!

Uh oh!

akoeplinger commented Apr 6, 2025

Uh oh!

am11 commented Apr 6, 2025

Uh oh!

akoeplinger commented Apr 6, 2025

Uh oh!

Uh oh!

davidwrighton commented Mar 8, 2025 •

edited

Loading