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.

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

Change the ReciprocalEstimate and ReciprocalSqrtEstimate APIs to be mustExpand on RyuJIT #102098

Merged
merged 17 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b2d559e
Change the ReciprocalEstimate and ReciprocalSqrtEstimate APIs to be m…
tannergooding May 10, 2024
30dc925
Apply formatting patch
tannergooding May 10, 2024
67a391b
Fix the RV64 and LA64 builds
tannergooding May 10, 2024
42a1dd5
Mark the ReciprocalEstimate and ReciprocalSqrtEstimate methods as Agg…
tannergooding May 11, 2024
f177bd2
Mark other usages of ReciprocalEstimate and ReciprocalSqrtEstimate in…
tannergooding May 11, 2024
62933bc
Mark several non-deterministic APIs as BypassReadyToRun and skip intr…
tannergooding May 11, 2024
afdcb1b
Cleanup based on PR recommendations to rely on the runtime rather tha…
tannergooding May 11, 2024
91d105c
Adding a regression test ensuring direct and indirect invocation of n…
tannergooding May 11, 2024
ed406bf
Add a note about non-deterministic intrinsic expansion to the botr
tannergooding May 11, 2024
1b16a09
Apply formatting patch
tannergooding May 11, 2024
1000066
Merge branch 'main' into fix-101731
tannergooding May 12, 2024
0312112
Ensure vector tests are correctly validating against the scalar imple…
tannergooding May 12, 2024
461c4b5
Fix the JIT/SIMD/VectorConvert test and workaround a 32-bit test issue
tannergooding May 12, 2024
9983d9d
Skip a test on Mono due to a known/tracked issue
tannergooding May 12, 2024
5bddfc1
Ensure that lowering on Arm64 doesn't make an assumption about cast s…
tannergooding May 12, 2024
1e0879f
Ensure the tier0opts local is used
tannergooding May 13, 2024
f63c575
Ensure impEstimateIntrinsic bails out for APIs that need to be implem…
tannergooding May 13, 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
56 changes: 48 additions & 8 deletions src/coreclr/jit/hwintrinsicarm64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -624,10 +624,20 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
break;
}

case NI_Vector64_ConvertToInt32:
case NI_Vector64_ConvertToInt32Native:
case NI_Vector128_ConvertToInt32:
case NI_Vector128_ConvertToInt32Native:
{
if (opts.IsReadyToRun())
Copy link
Member Author

Choose a reason for hiding this comment

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

I considered making this a flag in the hwintrinsic list table, to allow this to be more easily centralized for any future APIs that fit in this bucket, but opted not to since we don't have many spare bits for the that flags enum right now and this helps keep the cost lower for the main intrinsic path.

jkotas marked this conversation as resolved.
Show resolved Hide resolved
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}
FALLTHROUGH;
}

case NI_Vector64_ConvertToInt32:
case NI_Vector128_ConvertToInt32:
{
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_FLOAT);
Expand All @@ -637,10 +647,20 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
break;
}

case NI_Vector64_ConvertToInt64:
case NI_Vector64_ConvertToInt64Native:
case NI_Vector128_ConvertToInt64:
case NI_Vector128_ConvertToInt64Native:
{
if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}
FALLTHROUGH;
}

case NI_Vector64_ConvertToInt64:
case NI_Vector128_ConvertToInt64:
{
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_DOUBLE);
Expand All @@ -661,10 +681,20 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
break;
}

case NI_Vector64_ConvertToUInt32:
case NI_Vector64_ConvertToUInt32Native:
case NI_Vector128_ConvertToUInt32:
case NI_Vector128_ConvertToUInt32Native:
{
if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}
FALLTHROUGH;
}

case NI_Vector64_ConvertToUInt32:
case NI_Vector128_ConvertToUInt32:
{
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_FLOAT);
Expand All @@ -674,10 +704,20 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
break;
}

case NI_Vector64_ConvertToUInt64:
case NI_Vector64_ConvertToUInt64Native:
case NI_Vector128_ConvertToUInt64:
case NI_Vector128_ConvertToUInt64Native:
{
if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}
FALLTHROUGH;
}

case NI_Vector64_ConvertToUInt64:
case NI_Vector128_ConvertToUInt64:
{
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_DOUBLE);
Expand Down
29 changes: 29 additions & 0 deletions src/coreclr/jit/hwintrinsicxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1436,6 +1436,13 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_FLOAT);

if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}

op1 = impSIMDPopStack();
retNode = gtNewSimdCvtNativeNode(retType, op1, CORINFO_TYPE_INT, simdBaseJitType, simdSize);
break;
Expand Down Expand Up @@ -1463,6 +1470,13 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_DOUBLE);

if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}

if (IsBaselineVector512IsaSupportedOpportunistically())
{
op1 = impSIMDPopStack();
Expand Down Expand Up @@ -1542,6 +1556,13 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_FLOAT);

if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}

if (IsBaselineVector512IsaSupportedOpportunistically())
{
op1 = impSIMDPopStack();
Expand All @@ -1556,6 +1577,7 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
{
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_DOUBLE);

if (IsBaselineVector512IsaSupportedOpportunistically())
{
op1 = impSIMDPopStack();
Expand All @@ -1571,6 +1593,13 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
assert(sig->numArgs == 1);
assert(simdBaseType == TYP_DOUBLE);

if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
break;
}

if (IsBaselineVector512IsaSupportedOpportunistically())
{
op1 = impSIMDPopStack();
Expand Down
67 changes: 47 additions & 20 deletions src/coreclr/jit/importercalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3125,15 +3125,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
// To be fixed in https://github.com/dotnet/runtime/pull/77465
const bool tier0opts = !opts.compDbgCode && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT);

jkotas marked this conversation as resolved.
Show resolved Hide resolved
if (tier0opts)
{
// The *Estimate APIs are allowed to differ in behavior across hardware
// so ensure we treat them as "betterToExpand" to get deterministic behavior

betterToExpand |= (ni == NI_System_Math_ReciprocalEstimate);
betterToExpand |= (ni == NI_System_Math_ReciprocalSqrtEstimate);
}
else if (!mustExpand)
if (!mustExpand)
{
switch (ni)
{
Expand Down Expand Up @@ -5199,8 +5191,18 @@ GenTree* Compiler::impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic,

switch (intrinsic)
{
case NI_PRIMITIVE_ConvertToInteger:
case NI_PRIMITIVE_ConvertToIntegerNative:
{
if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
return nullptr;
}
jkotas marked this conversation as resolved.
Show resolved Hide resolved
FALLTHROUGH;
}

case NI_PRIMITIVE_ConvertToInteger:
{
assert(sig->sigInst.methInstCount == 1);
assert(varTypeIsFloating(baseType));
Expand Down Expand Up @@ -7471,6 +7473,8 @@ bool Compiler::IsTargetIntrinsic(NamedIntrinsic intrinsicName)
{
case NI_System_Math_Abs:
case NI_System_Math_Sqrt:
case NI_System_Math_ReciprocalEstimate:
case NI_System_Math_ReciprocalSqrtEstimate:
return true;

default:
Expand Down Expand Up @@ -8771,10 +8775,16 @@ GenTree* Compiler::impEstimateIntrinsic(CORINFO_METHOD_HANDLE method,
assert(varTypeIsFloating(callType));
assert(sig->numArgs == 1);

if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
return nullptr;
}

#if defined(FEATURE_HW_INTRINSICS)
// We use compExactlyDependsOn since these are estimate APIs where
// the behavior is explicitly allowed to differ across machines and
// we want to ensure that it gets marked as such in R2R.
// the behavior is explicitly allowed to differ across machines

var_types simdType = TYP_UNKNOWN;
NamedIntrinsic intrinsicId = NI_Illegal;
Expand Down Expand Up @@ -8847,7 +8857,7 @@ GenTree* Compiler::impEstimateIntrinsic(CORINFO_METHOD_HANDLE method,
simdSize = 16;
}

GenTree* op1 = impPopStack().val;
GenTree* op1 = impImplicitR4orR8Cast(impPopStack().val, callType);

op1 = gtNewSimdCreateScalarUnsafeNode(simdType, op1, callJitType, simdSize);
op1 = gtNewSimdHWIntrinsicNode(simdType, op1, intrinsicId, callJitType, simdSize);
Expand All @@ -8856,10 +8866,27 @@ GenTree* Compiler::impEstimateIntrinsic(CORINFO_METHOD_HANDLE method,
}
#endif // FEATURE_HW_INTRINSICS

// TODO-CQ: Returning this as an intrinsic blocks inlining and is undesirable
// return impMathIntrinsic(method, sig, callType, intrinsicName, tailCall);
switch (intrinsicName)
{
case NI_System_Math_ReciprocalEstimate:
case NI_System_Math_ReciprocalSqrtEstimate:
{
GenTree* op1 = impImplicitR4orR8Cast(impPopStack().val, callType);

return nullptr;
if (intrinsicName == NI_System_Math_ReciprocalSqrtEstimate)
{
op1 = new (this, GT_INTRINSIC)
GenTreeIntrinsic(genActualType(callType), op1, NI_System_Math_Sqrt, nullptr);
}

return gtNewOperNode(GT_DIV, genActualType(callType), gtNewDconNode(1.0, callType), op1);
}

default:
{
unreached();
}
}
}

GenTree* Compiler::impMathIntrinsic(CORINFO_METHOD_HANDLE method,
Expand Down Expand Up @@ -8958,8 +8985,8 @@ GenTree* Compiler::impMinMaxIntrinsic(CORINFO_METHOD_HANDLE method,
GenTreeDblCon* cnsNode = nullptr;
GenTree* otherNode = nullptr;

GenTree* op2 = impStackTop().val;
GenTree* op1 = impStackTop(1).val;
GenTree* op2 = impImplicitR4orR8Cast(impStackTop().val, callType);
GenTree* op1 = impImplicitR4orR8Cast(impStackTop(1).val, callType);

if (op2->IsCnsFltOrDbl())
{
Expand Down Expand Up @@ -9227,7 +9254,7 @@ GenTree* Compiler::impMinMaxIntrinsic(CORINFO_METHOD_HANDLE method,
retNode->AsHWIntrinsic()->Op(2) = op1;
}

return gtNewSimdToScalarNode(callType, retNode, callJitType, 16);
return gtNewSimdToScalarNode(genActualType(callType), retNode, callJitType, 16);
}
}
#endif // FEATURE_HW_INTRINSICS && TARGET_XARCH
Expand Down Expand Up @@ -9392,7 +9419,7 @@ GenTree* Compiler::impMinMaxIntrinsic(CORINFO_METHOD_HANDLE method,
callJitType, 16);
}

return gtNewSimdToScalarNode(callType, tmp, callJitType, 16);
return gtNewSimdToScalarNode(genActualType(callType), tmp, callJitType, 16);
}
#endif // FEATURE_HW_INTRINSICS && TARGET_XARCH

Expand Down
34 changes: 31 additions & 3 deletions src/coreclr/jit/simdashwintrinsic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -512,8 +512,39 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,

switch (intrinsic)
{
case NI_VectorT_ConvertToInt32Native:
{
if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
return nullptr;
}
break;
}

case NI_VectorT_ConvertToInt64Native:
case NI_VectorT_ConvertToUInt32Native:
case NI_VectorT_ConvertToUInt64Native:
{
if (opts.IsReadyToRun())
{
// We explicitly block these APIs from being expanded in R2R
// since we know they are non-deterministic across hardware
return nullptr;
}

#if defined(TARGET_XARCH)
if (!IsBaselineVector512IsaSupportedOpportunistically())
{
return nullptr;
}
#endif // TARGET_XARCH

break;
}

#if defined(TARGET_XARCH)
case NI_VectorT_ConvertToDouble:
{
if (IsBaselineVector512IsaSupportedOpportunistically())
Expand All @@ -533,11 +564,8 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,
}

case NI_VectorT_ConvertToInt64:
case NI_VectorT_ConvertToInt64Native:
case NI_VectorT_ConvertToUInt32:
case NI_VectorT_ConvertToUInt32Native:
case NI_VectorT_ConvertToUInt64:
case NI_VectorT_ConvertToUInt64Native:
{
if (IsBaselineVector512IsaSupportedOpportunistically())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Resources\SatelliteContractVersionAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Resources\UltimateResourceFallbackLocation.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\AmbiguousImplementationException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\BypassReadyToRunAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\CompilerServices\AccessedThroughPropertyAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\CompilerServices\AsyncIteratorMethodBuilder.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\CompilerServices\AsyncIteratorStateMachineAttribute.cs" />
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Double.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Numerics;
using System.Runtime;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
Expand Down Expand Up @@ -662,6 +663,7 @@ public static TInteger ConvertToInteger<TInteger>(double value)

/// <inheritdoc cref="IFloatingPoint{TSelf}.ConvertToIntegerNative{TInteger}(TSelf)" />
[Intrinsic]
[BypassReadyToRun]
public static TInteger ConvertToIntegerNative<TInteger>(double value)
where TInteger : IBinaryInteger<TInteger> => TInteger.CreateSaturating(value);

Expand Down
Loading
Loading