Refactor handling of two immediates in hwintrinsic #102071
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
a74nh
force-pushed
the
immediate_refactor_github
branch
from
1fc5007
to
b04cc90
Compare
a74nh
force-pushed
the
immediate_refactor_github
branch
from
b04cc90
to
cd42382
Compare
|
@kunalspathak @tannergooding @dotnet/arm64-contrib This is ready. A perquisite for |
| @@ -221,7 +355,7 @@ bool HWIntrinsicInfo::isScalarIsa(CORINFO_InstructionSet isa) | |||
| // pImmUpperBound [OUT] - The upper incl. bound for a value of the intrinsic immediate operand | |||
| // | |||
| void HWIntrinsicInfo::lookupImmBounds( | |||
| NamedIntrinsic intrinsic, int simdSize, var_types baseType, int* pImmLowerBound, int* pImmUpperBound) | |||
| NamedIntrinsic intrinsic, int simdSize, var_types baseType, int immNumber, int* pImmLowerBound, int* pImmUpperBound) | |||
There was a problem hiding this comment.
Why is this method taking immNumber? It doesn't look like its being used anywhere...
There was a problem hiding this comment.
That's because we don't yet have any intrinsics with two immediates. Next PR will add:
case NI_Sve_SaturatingDecrementBy16BitElementCount:
case NI_Sve_SaturatingDecrementBy32BitElementCount:
..etc...
case NI_Sve_SaturatingIncrementBy64BitElementCountVector:
case NI_Sve_SaturatingIncrementBy8BitElementCountVector:
if (immNumber == 1)
{
immLowerBound = 1;
immUpperBound = 16;
}
else
{
assert(immNumber == 2);
immLowerBound = (int)SVE_PATTERN_POW2;
immUpperBound = (int)SVE_PATTERN_ALL;
}
break;
There was a problem hiding this comment.
worth adding the comment about the new argument in method docs. ok to do it in follow-up PR when you add cases to handle NI_Sve_SaturatingDecrementBy*
| *immSimdBaseType = JitType2PreciseVarType(otherBaseJitType); | ||
| assert(otherBaseJitType == simdBaseJitType); | ||
| } | ||
| // For imm1 use default simd sizes. |
There was a problem hiding this comment.
Should we be asserting here instead? It seems like there's a presumption about the value of immSimdBaseType and that the caller essentially shouldn't call this function for immNumber == 1 for InsertSelectedScalar
There was a problem hiding this comment.
The code isn't clear here.
The immSimdSize and immSimdBaseType passed in are the default size and type for the intrinsic. The function can then choose to override them (as it does for HW_Category_SIMDByIndexedElement and imm2 of InsertSelectedScalar). For all other intrinsics (including imm1 of InsertSelectedScalar) it does nothing so that the default size and type can be used.
Updated the block comment so that it's clearer.
| @@ -221,7 +355,7 @@ bool HWIntrinsicInfo::isScalarIsa(CORINFO_InstructionSet isa) | |||
| // pImmUpperBound [OUT] - The upper incl. bound for a value of the intrinsic immediate operand | |||
| // | |||
| void HWIntrinsicInfo::lookupImmBounds( | |||
| NamedIntrinsic intrinsic, int simdSize, var_types baseType, int* pImmLowerBound, int* pImmUpperBound) | |||
| NamedIntrinsic intrinsic, int simdSize, var_types baseType, int immNumber, int* pImmLowerBound, int* pImmUpperBound) | |||
There was a problem hiding this comment.
worth adding the comment about the new argument in method docs. ok to do it in follow-up PR when you add cases to handle NI_Sve_SaturatingDecrementBy*
|
/azp run runtime-coreclr superpmi-replay, runtime-coreclr superpmi-diffs |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Out of all the existing hwintrinsics, only
NI_AdvSimd_Arm64_InsertSelectedScalarhas two immediate values. This also has the property that both immediates are of the same type, therefore handling of the type information and type checking can be simplified for the second immediate.For Sve,
SaturatingIncrementBy*ElementCount,SaturatingDecrementBy*ElementCount, andMultiplyAddRotateComplexBySelectedScalarall require two immediate values. In addition for the Saturating functions, the immediates have different types.This PR refactors the existing handling so that it will work for Sve. It does not add any new Sve intrinsics.
The code still feels a little clunky, but I hope
impHWIntrinsic()is much easier to parse now.