Skip to content
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

Rewrite math jit helpers to managed code #98858

Closed
wants to merge 36 commits into from
Closed

Rewrite math jit helpers to managed code #98858

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
5968f8d
Rewrite math jit helpers to managed code
MichalPetryka Feb 23, 2024
dfbcf22
Fix asserts
MichalPetryka Feb 23, 2024
c665079
Merge branch 'main' into math-helpers
MichalPetryka Feb 25, 2024
d150f78
Merge branch 'main' into math-helpers
MichalPetryka Feb 29, 2024
49c110d
Merge remote-tracking branch 'upstream/main' into math-helpers
MichalPetryka Mar 6, 2024
878b573
Cleanup changes, add comments
MichalPetryka Mar 6, 2024
ef4868b
Fix typo
MichalPetryka Mar 6, 2024
08eaa08
Fix small type overflow in the scanner
MichalPetryka Mar 7, 2024
a96f68a
Update JIT-EE GUID
MichalPetryka Mar 7, 2024
2b4b795
Remove helper
MichalPetryka Mar 7, 2024
06cd46c
Merge remote-tracking branch 'upstream/main' into math-helpers
MichalPetryka Mar 9, 2024
64b9bbc
Fully managed conversions to double
MichalPetryka Mar 10, 2024
769d5eb
Merge remote-tracking branch 'upstream/main' into math-helpers
MichalPetryka Mar 11, 2024
b4d0521
Restore one ASM helper
MichalPetryka Mar 11, 2024
ab17973
Merge remote-tracking branch 'upstream/main' into math-helpers
MichalPetryka Mar 13, 2024
6a525ee
Cleanup code, move uint cast to managed
MichalPetryka Mar 13, 2024
da0b434
Update jithelpers.h
MichalPetryka Mar 13, 2024
18565e4
Remove DoubleToUInt
MichalPetryka Mar 13, 2024
a7e114b
Move the helpers
MichalPetryka Mar 13, 2024
070bead
Remove the using
MichalPetryka Mar 13, 2024
d4fd66d
Fix typos
MichalPetryka Mar 13, 2024
8efe9eb
Rename defined and helper
MichalPetryka Mar 14, 2024
9c97f32
Merge remote-tracking branch 'upstream/main' into math-helpers
MichalPetryka Mar 14, 2024
7943ef7
Update jitinterface.h
MichalPetryka Mar 14, 2024
334838b
Rename to Checked
MichalPetryka Mar 14, 2024
1cdcf5e
Cleanup NativeAOT, fix parameter names
MichalPetryka Mar 14, 2024
e221809
Update jitinterface.cpp
MichalPetryka Mar 14, 2024
2188fe8
Cleanup throw helpers
MichalPetryka Mar 14, 2024
b4a5511
Remove the unmanaged helper from the tiering optimization
MichalPetryka Mar 15, 2024
ffabbe7
Go back to C++ helper for doubles, root things with scanner, add fmod
MichalPetryka Mar 15, 2024
68c39a4
Update MathHelpers.cpp
MichalPetryka Mar 15, 2024
02c5335
Move unused helper back to C++, restore scanner ifs
MichalPetryka Mar 15, 2024
1c36262
Update ILImporter.Scanner.cs
MichalPetryka Mar 15, 2024
8fcb99f
Update ILImporter.Scanner.cs
MichalPetryka Mar 15, 2024
f7769d7
Update src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanne…
MichalPetryka Mar 15, 2024
be4c607
Call FMod directly
MichalPetryka Mar 16, 2024
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
10 changes: 5 additions & 5 deletions src/coreclr/inc/corinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,18 +387,18 @@ enum CorInfoHelpFunc
CORINFO_HELP_ULMOD,
CORINFO_HELP_LNG2DBL, // Convert a signed int64 to a double
CORINFO_HELP_ULNG2DBL, // Convert a unsigned int64 to a double
CORINFO_HELP_DBL2INT,
CORINFO_HELP_DBL2INT, // unused
MichalPetryka marked this conversation as resolved.
Show resolved Hide resolved
CORINFO_HELP_DBL2INT_OVF,
CORINFO_HELP_DBL2LNG,
CORINFO_HELP_DBL2LNG_OVF,
CORINFO_HELP_DBL2UINT,
CORINFO_HELP_DBL2UINT, // unused
MichalPetryka marked this conversation as resolved.
Show resolved Hide resolved
MichalPetryka marked this conversation as resolved.
Show resolved Hide resolved
CORINFO_HELP_DBL2UINT_OVF,
CORINFO_HELP_DBL2ULNG,
CORINFO_HELP_DBL2ULNG_OVF,
CORINFO_HELP_FLTREM,
CORINFO_HELP_DBLREM,
CORINFO_HELP_FLTROUND,
CORINFO_HELP_DBLROUND,
CORINFO_HELP_FLTROUND, // unused
CORINFO_HELP_DBLROUND, // unused

/* Allocating a new object. Always use ICorClassInfo::getNewHelper() to decide
which is the right helper to use to allocate an object of a given type. */
Expand Down Expand Up @@ -2061,7 +2061,7 @@ class ICorStaticInfo
// Example of a scenario addressed by notifyMethodInfoUsage:
// 1) Crossgen (with --opt-cross-module=MyLib) attempts to inline a call from MyLib.dll into MyApp.dll
// and realizes that the call always throws.
// 2) JIT aborts the inlining attempt and marks the call as no-return instead. The code that follows the call is
// 2) JIT aborts the inlining attempt and marks the call as no-return instead. The code that follows the call is
// replaced with a breakpoint instruction that is expected to be unreachable.
// 3) MyLib is updated to a new version so it's no longer within the same version bubble with MyApp.dll
// and the new version of the call no longer throws and does some work.
Expand Down
34 changes: 16 additions & 18 deletions src/coreclr/inc/jithelpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@
JITHELPER(CORINFO_HELP_UDIV, JIT_UDiv, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_UMOD, JIT_UMod, CORINFO_HELP_SIG_8_STACK)

// CORINFO_HELP_DBL2INT, CORINFO_HELP_DBL2UINT, and CORINFO_HELP_DBL2LONG get
// patched for CPUs that support SSE2 (P4 and above).
#ifndef TARGET_64BIT
JITHELPER(CORINFO_HELP_LLSH, JIT_LLsh, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_LRSH, JIT_LRsh, CORINFO_HELP_SIG_REG_ONLY)
Expand All @@ -47,26 +45,26 @@
JITHELPER(CORINFO_HELP_LRSZ, NULL, CORINFO_HELP_SIG_CANNOT_USE_ALIGN_STUB)
#endif // TARGET_64BIT
JITHELPER(CORINFO_HELP_LMUL, JIT_LMul, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_LMUL_OVF, JIT_LMulOvf, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_ULMUL_OVF, JIT_ULMulOvf, CORINFO_HELP_SIG_16_STACK)
DYNAMICJITHELPER(CORINFO_HELP_LMUL_OVF, NULL, CORINFO_HELP_SIG_16_STACK)
MichalPetryka marked this conversation as resolved.
Show resolved Hide resolved
DYNAMICJITHELPER(CORINFO_HELP_ULMUL_OVF, NULL, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_LDIV, JIT_LDiv, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_LMOD, JIT_LMod, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_ULDIV, JIT_ULDiv, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_ULMOD, JIT_ULMod, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_LNG2DBL, JIT_Lng2Dbl, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_ULNG2DBL, JIT_ULng2Dbl, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2INT, JIT_Dbl2Lng, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBL2INT_OVF, JIT_Dbl2IntOvf, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2LNG, JIT_Dbl2Lng, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBL2LNG_OVF, JIT_Dbl2LngOvf, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2UINT, JIT_Dbl2Lng, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBL2UINT_OVF, JIT_Dbl2UIntOvf, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBL2ULNG, JIT_Dbl2ULng, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBL2ULNG_OVF, JIT_Dbl2ULngOvf, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_FLTREM, JIT_FltRem, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBLREM, JIT_DblRem, CORINFO_HELP_SIG_16_STACK)
JITHELPER(CORINFO_HELP_FLTROUND, JIT_FloatRound, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBLROUND, JIT_DoubleRound, CORINFO_HELP_SIG_16_STACK)
DYNAMICJITHELPER(CORINFO_HELP_ULNG2DBL, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2INT, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2INT_OVF, NULL, CORINFO_HELP_SIG_8_STACK)
JITHELPER(CORINFO_HELP_DBL2LNG, JIT_Dbl2Lng, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2LNG_OVF, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2UINT, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2UINT_OVF, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2ULNG, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBL2ULNG_OVF, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_FLTREM, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBLREM, NULL, CORINFO_HELP_SIG_16_STACK)
DYNAMICJITHELPER(CORINFO_HELP_FLTROUND, NULL, CORINFO_HELP_SIG_8_STACK)
DYNAMICJITHELPER(CORINFO_HELP_DBLROUND, NULL, CORINFO_HELP_SIG_16_STACK)

// Allocating a new object
JITHELPER(CORINFO_HELP_NEWFAST, JIT_New, CORINFO_HELP_SIG_REG_ONLY)
Expand Down Expand Up @@ -203,7 +201,7 @@
JITHELPER(CORINFO_HELP_GETSHARED_NONGCTHREADSTATIC_BASE, JIT_GetSharedNonGCThreadStaticBase, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_GETSHARED_GCTHREADSTATIC_BASE_NOCTOR, JIT_GetSharedGCThreadStaticBase, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_GETSHARED_NONGCTHREADSTATIC_BASE_NOCTOR, JIT_GetSharedNonGCThreadStaticBase, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_GETSHARED_GCTHREADSTATIC_BASE_DYNAMICCLASS, JIT_GetSharedGCThreadStaticBaseDynamicClass, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_GETSHARED_GCTHREADSTATIC_BASE_DYNAMICCLASS, JIT_GetSharedGCThreadStaticBaseDynamicClass, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_GETSHARED_NONGCTHREADSTATIC_BASE_DYNAMICCLASS, JIT_GetSharedNonGCThreadStaticBaseDynamicClass, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_GETSHARED_GCTHREADSTATIC_BASE_NOCTOR_OPTIMIZED, JIT_GetSharedGCThreadStaticBaseOptimized, CORINFO_HELP_SIG_REG_ONLY)
JITHELPER(CORINFO_HELP_GETSHARED_NONGCTHREADSTATIC_BASE_NOCTOR_OPTIMIZED, JIT_GetSharedNonGCThreadStaticBaseOptimized, CORINFO_HELP_SIG_REG_ONLY)
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5969,6 +5969,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
switch (tree->AsIntrinsic()->gtIntrinsicName)
{
case NI_System_Math_Atan2:
case NI_System_Math_FMod:
jkotas marked this conversation as resolved.
Show resolved Hide resolved
case NI_System_Math_Pow:
// These math intrinsics are actually implemented by user calls. Increase the
// Sethi 'complexity' by two to reflect the argument register requirement.
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,9 @@ GenTree* Compiler::fgMorphExpandCast(GenTreeCast* tree)
#if defined(TARGET_ARM) || defined(TARGET_AMD64)
return nullptr;
#else // TARGET_X86
return fgMorphCastIntoHelper(tree, CORINFO_HELP_DBL2UINT, oper);
oper = gtNewCastNode(TYP_LONG, oper, false, TYP_LONG);
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to leave this change to the Intel PRs too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous behaviour here was kinda a bug since CORINFO_HELP_DBL2UINT was bound to Dbl2Lng, it only worked fine cause X86 and ARM32 ABIs return the lower bits of the long same as an uint. Calling the Dbl2UInt helper here would be a performance regression which you said are better left to the Intel PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree the existing code is a hack but there is no observable bug. There is no point in cleaning it up in this PR. It requires an extra throw away work to evaluate the impact. I suspect that this change may be changing the observable behavior. We want to have the changes of observable behavior centralized in the Intel PR.

This PR is 95% good and non-controversial, and it is just these few changes that are holding it back. If you would like it to merged soon, I would recommend reverting the few controversial changes that are holding it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect that this change may be changing the observable behavior.

How so? Isn't casting a long to uint the same as taking the lower 4B from it?

these few changes

You mean the change here and the // unused comments?

Copy link
Member

Choose a reason for hiding this comment

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

How so? Isn't casting a long to uint the same as taking the lower 4B from it?

This is casting double to uint. This sort of change can be changing behavior in the overflow case. I do not know. I do not want to spend any time reviewing changes related to floating point to integer conversion in this PR. It would be throw-away work.

these few changes

Ideally, anything related to floating point to integer conversions that are being completely redone in the Intel PR.

Copy link
Contributor Author

@MichalPetryka MichalPetryka Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, anything related to floating point to integer conversions that are being completely redone in the Intel PR.

I'd argue that cleaning things up here makes the Intel changes simpler since it reduces the amount of work they need to do there. If the Intel folks prefer it, I can remove those from here however cc @khushal1996

To avoid JIT changes in this PR, I've moved double -> uint casts to a managed implementation that copies what LLVM does in the latest commit since it's faster on my machine anyway. It is a bit of a throwaway work like you said but this change makes the PR simpler and I'd argue that it's safe since LLVM uses this implementation. Also, I don't think the Intel PR will have some better implementation for the helper here, it'll just add overflow checks to it.
Nevermind, the LLVM implementation returns different values for out of range doubles, gonna revert it then. It should still be safe to use with overflow checks though I think.

tree = gtNewCastNode(TYP_INT, oper, false, TYP_UINT);
return fgMorphTree(tree);
#endif // TARGET_X86

case TYP_LONG:
Expand Down
86 changes: 3 additions & 83 deletions src/coreclr/nativeaot/Runtime/MathHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,78 +5,13 @@
#include "CommonMacros.h"
#include "rhassert.h"

//
// Floating point and 64-bit integer math helpers.
//

EXTERN_C NATIVEAOT_API uint64_t REDHAWK_CALLCONV RhpDbl2ULng(double val)
{
const double two63 = 2147483648.0 * 4294967296.0;
uint64_t ret;
if (val < two63)
{
ret = (int64_t)(val);
}
else
{
// subtract 0x8000000000000000, do the convert then add it back again
ret = (int64_t)(val - two63) + I64(0x8000000000000000);
}
return ret;
}

#undef min
#undef max
#include <cmath>

EXTERN_C NATIVEAOT_API float REDHAWK_CALLCONV RhpFltRem(float dividend, float divisor)
{
//
// From the ECMA standard:
//
// If [divisor] is zero or [dividend] is infinity
// the result is NaN.
// If [divisor] is infinity,
// the result is [dividend] (negated for -infinity***).
//
// ***"negated for -infinity" has been removed from the spec
//

if (divisor==0 || !std::isfinite(dividend))
{
return -nanf("");
}
else if (!std::isfinite(divisor) && !std::isnan(divisor))
{
return dividend;
}
// else...
return fmodf(dividend,divisor);
}

EXTERN_C NATIVEAOT_API double REDHAWK_CALLCONV RhpDblRem(double dividend, double divisor)
{
//
// From the ECMA standard:
//
// If [divisor] is zero or [dividend] is infinity
// the result is NaN.
// If [divisor] is infinity,
// the result is [dividend] (negated for -infinity***).
//
// ***"negated for -infinity" has been removed from the spec
//
if (divisor==0 || !std::isfinite(dividend))
{
return -nan("");
}
else if (!std::isfinite(divisor) && !std::isnan(divisor))
{
return dividend;
}
// else...
return(fmod(dividend,divisor));
}
//
// Floating point and 64-bit integer math helpers.
//

#ifdef HOST_ARM
EXTERN_C NATIVEAOT_API int32_t REDHAWK_CALLCONV RhpIDiv(int32_t i, int32_t j)
Expand Down Expand Up @@ -152,24 +87,9 @@ EXTERN_C NATIVEAOT_API int64_t REDHAWK_CALLCONV RhpDbl2Lng(double val)
return (int64_t)val;
}

EXTERN_C NATIVEAOT_API int32_t REDHAWK_CALLCONV RhpDbl2Int(double val)
{
return (int32_t)val;
}

EXTERN_C NATIVEAOT_API uint32_t REDHAWK_CALLCONV RhpDbl2UInt(double val)
{
return (uint32_t)val;
}

EXTERN_C NATIVEAOT_API double REDHAWK_CALLCONV RhpLng2Dbl(int64_t val)
{
return (double)val;
}

EXTERN_C NATIVEAOT_API double REDHAWK_CALLCONV RhpULng2Dbl(uint64_t val)
{
return (double)val;
}

#endif // HOST_ARM
Loading