Skip to content

Commit

Permalink
JIT: Encode SIMD base type in VN for all HW intrinsics (#105869)
Browse files Browse the repository at this point in the history 
This changes the VN funcs for all HW intrinsic functions to always have an extra
parameter for the SIMD base type. Before this change it was based on the
instruction list, however that is not always the right thing (e.g. #105721 is a
bug because of that). We also kept it as part of the HW intrinsic table, but
that requires manual maintenance and is easy to get wrong. Always encoding the
type is much more simple and the diffs do not look too bad.

In .NET 10 we can decide if we want to opt some intrinsics into not being
differentiated based on the SIMD base type. The easiest thing might be to always
map those to have the same base SIMD type (e.g. `CORINFO_TYPE_BYTE`) so that we
don't end up with differences in arities for some VN functions that are hard to
reason about.

Fix #105721
  • Loading branch information
@jakobbotsch @pull
jakobbotsch authored and pull[bot] committed Aug 15, 2024
1 parent 65984f9 commit 1524546
Show file tree
Hide file tree
Showing 11 changed files with 2,553 additions and 2,663 deletions.
4 changes: 0 additions & 4 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9531,10 +9531,6 @@ class Compiler
#endif // FEATURE_SIMD
}

#ifdef FEATURE_HW_INTRINSICS
static bool vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID);
#endif // FEATURE_HW_INTRINSICS

private:
// Returns true if the TYP_SIMD locals on stack are aligned at their
// preferred byte boundary specified by getSIMDTypeAlignment().
Expand Down
73 changes: 2 additions & 71 deletions src/coreclr/jit/hwintrinsic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
static const HWIntrinsicInfo hwIntrinsicInfoArray[] = {
// clang-format off
#if defined(TARGET_XARCH)
#define HARDWARE_INTRINSIC(isa, name, size, numarg, extra, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
#define HARDWARE_INTRINSIC(isa, name, size, numarg, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
{ \
/* name */ #name, \
/* flags */ static_cast<HWIntrinsicFlag>(flag), \
Expand All @@ -22,7 +22,7 @@ static const HWIntrinsicInfo hwIntrinsicInfoArray[] = {
},
#include "hwintrinsiclistxarch.h"
#elif defined (TARGET_ARM64)
#define HARDWARE_INTRINSIC(isa, name, size, numarg, extra, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
#define HARDWARE_INTRINSIC(isa, name, size, numarg, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
{ \
/* name */ #name, \
/* flags */ static_cast<HWIntrinsicFlag>(flag), \
Expand Down Expand Up @@ -759,75 +759,6 @@ CorInfoType Compiler::getBaseJitTypeFromArgIfNeeded(NamedIntrinsic intrins
return simdBaseJitType;
}

//------------------------------------------------------------------------
// vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID):
//
// Arguments:
// hwIntrinsicID -- The id for the HW intrinsic
//
// Return Value:
// Returns true if this intrinsic requires value numbering to add an
// extra SimdType argument that encodes the resulting type.
// If we don't do this overloaded versions can return the same VN
// leading to incorrect CSE substitutions.
//
/* static */ bool Compiler::vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID)
{
// No extra type information is needed for scalar/special HW Intrinsic.
//
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
return false;
}

int numArgs = HWIntrinsicInfo::lookupNumArgs(hwIntrinsicID);

// HW Intrinsic's with -1 for numArgs have a varying number of args, so we currently
// give them a unique value number, and don't add an extra argument.
//
if (numArgs == -1)
{
return false;
}

// We iterate over all of the different baseType's for this intrinsic in the HWIntrinsicInfo table
// We set diffInsCount to the number of instructions that can execute differently.
//
unsigned diffInsCount = 0;
#ifdef TARGET_XARCH
instruction lastIns = INS_invalid;
#endif
for (var_types baseType = TYP_BYTE; (baseType <= TYP_DOUBLE); baseType = (var_types)(baseType + 1))
{
instruction curIns = HWIntrinsicInfo::lookupIns(hwIntrinsicID, baseType);
if (curIns != INS_invalid)
{
#ifdef TARGET_XARCH
if (curIns != lastIns)
{
diffInsCount++;
// remember the last valid instruction that we saw
lastIns = curIns;
}
#elif defined(TARGET_ARM64)
// On ARM64 we use the same instruction and specify an insOpt arrangement
// so we always consider the instruction operation to be different
//
diffInsCount++;
#endif // TARGET
if (diffInsCount >= 2)
{
// We can early exit the loop now
break;
}
}
}

// If we see two (or more) different instructions we need the extra VNF_SimdType arg
return (diffInsCount >= 2);
}

struct HWIntrinsicIsaRange
{
NamedIntrinsic FirstId;
Expand Down
1,446 changes: 723 additions & 723 deletions src/coreclr/jit/hwintrinsiclistarm64.h
View file

Large diffs are not rendered by default.

570 changes: 285 additions & 285 deletions src/coreclr/jit/hwintrinsiclistarm64sve.h
View file

Large diffs are not rendered by default.

2,622 changes: 1,310 additions & 1,312 deletions src/coreclr/jit/hwintrinsiclistxarch.h
View file

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions src/coreclr/jit/namedintrinsiclist.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,11 @@ enum NamedIntrinsic : unsigned short
#ifdef FEATURE_HW_INTRINSICS
NI_HW_INTRINSIC_START,
#if defined(TARGET_XARCH)
#define HARDWARE_INTRINSIC(isa, name, size, numarg, extra, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
#define HARDWARE_INTRINSIC(isa, name, size, numarg, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
NI_##isa##_##name,
#include "hwintrinsiclistxarch.h"
#elif defined(TARGET_ARM64)
#define HARDWARE_INTRINSIC(isa, name, size, numarg, extra, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
#define HARDWARE_INTRINSIC(isa, name, size, numarg, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
NI_##isa##_##name,
#include "hwintrinsiclistarm64.h"
#endif // !defined(TARGET_XARCH) && !defined(TARGET_ARM64)
Expand Down
143 changes: 36 additions & 107 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2947,7 +2947,8 @@ ValueNum ValueNumStore::VNForFunc(
assert(arg1VN == VNNormalValue(arg1VN));
assert(arg2VN == VNNormalValue(arg2VN));
assert((func == VNF_MapStore) || (arg3VN == VNNormalValue(arg3VN)));
assert(VNFuncArity(func) == 4);
// Some SIMD functions with variable number of arguments are defined with zero arity
assert((VNFuncArity(func) == 0) || (VNFuncArity(func) == 4));

// Have we already assigned a ValueNum for 'func'('arg0VN','arg1VN','arg2VN','arg3VN') ?
//
Expand Down Expand Up @@ -7823,8 +7824,10 @@ ValueNum EvaluateSimdCvtVectorToMask(ValueNumStore* vns, var_types simdType, var
return vns->VNForSimdMaskCon(result);
}

ValueNum ValueNumStore::EvalHWIntrinsicFunUnary(
GenTreeHWIntrinsic* tree, VNFunc func, ValueNum arg0VN, bool encodeResultType, ValueNum resultTypeVN)
ValueNum ValueNumStore::EvalHWIntrinsicFunUnary(GenTreeHWIntrinsic* tree,
VNFunc func,
ValueNum arg0VN,
ValueNum resultTypeVN)
{
var_types type = tree->TypeGet();
var_types baseType = tree->GetSimdBaseType();
Expand Down Expand Up @@ -8158,19 +8161,11 @@ ValueNum ValueNumStore::EvalHWIntrinsicFunUnary(
}
}

if (encodeResultType)
{
return VNForFunc(type, func, arg0VN, resultTypeVN);
}
return VNForFunc(type, func, arg0VN);
return VNForFunc(type, func, arg0VN, resultTypeVN);
}

ValueNum ValueNumStore::EvalHWIntrinsicFunBinary(GenTreeHWIntrinsic* tree,
VNFunc func,
ValueNum arg0VN,
ValueNum arg1VN,
bool encodeResultType,
ValueNum resultTypeVN)
ValueNum ValueNumStore::EvalHWIntrinsicFunBinary(
GenTreeHWIntrinsic* tree, VNFunc func, ValueNum arg0VN, ValueNum arg1VN, ValueNum resultTypeVN)
{
var_types type = tree->TypeGet();
var_types baseType = tree->GetSimdBaseType();
Expand Down Expand Up @@ -8953,11 +8948,7 @@ ValueNum ValueNumStore::EvalHWIntrinsicFunBinary(GenTreeHWIntrinsic* tree,
}
}

if (encodeResultType)
{
return VNForFunc(type, func, arg0VN, arg1VN, resultTypeVN);
}
return VNForFunc(type, func, arg0VN, arg1VN);
return VNForFunc(type, func, arg0VN, arg1VN, resultTypeVN);
}

ValueNum EvaluateSimdWithElementFloating(
Expand Down Expand Up @@ -9068,13 +9059,8 @@ ValueNum EvaluateSimdWithElementIntegral(
}
}

ValueNum ValueNumStore::EvalHWIntrinsicFunTernary(GenTreeHWIntrinsic* tree,
VNFunc func,
ValueNum arg0VN,
ValueNum arg1VN,
ValueNum arg2VN,
bool encodeResultType,
ValueNum resultTypeVN)
ValueNum ValueNumStore::EvalHWIntrinsicFunTernary(
GenTreeHWIntrinsic* tree, VNFunc func, ValueNum arg0VN, ValueNum arg1VN, ValueNum arg2VN, ValueNum resultTypeVN)
{
var_types type = tree->TypeGet();
var_types baseType = tree->GetSimdBaseType();
Expand Down Expand Up @@ -9185,14 +9171,7 @@ ValueNum ValueNumStore::EvalHWIntrinsicFunTernary(GenTreeHWIntrinsic* tree,
}
}

if (encodeResultType)
{
return VNForFunc(type, func, arg0VN, arg1VN, arg2VN, resultTypeVN);
}
else
{
return VNForFunc(type, func, arg0VN, arg1VN, arg2VN);
}
return VNForFunc(type, func, arg0VN, arg1VN, arg2VN, resultTypeVN);
}

#endif // FEATURE_HW_INTRINSICS
Expand Down Expand Up @@ -10381,25 +10360,6 @@ void ValueNumStore::vnDumpZeroObj(Compiler* comp, VNFuncApp* zeroObj)

// Static fields, methods.

#define ValueNumFuncDef(vnf, arity, commute, knownNonNull, sharedStatic, extra) \
static_assert((arity) >= 0 || !(extra), "valuenumfuncs.h has EncodesExtraTypeArg==true and arity<0 for " #vnf);
#include "valuenumfuncs.h"

#ifdef FEATURE_HW_INTRINSICS

#define HARDWARE_INTRINSIC(isa, name, size, argCount, extra, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10, category, flag) \
static_assert((size) != 0 || !(extra), \
"hwintrinsicslist<arch>.h has EncodesExtraTypeArg==true and size==0 for " #isa " " #name);
#if defined(TARGET_XARCH)
#include "hwintrinsiclistxarch.h"
#elif defined(TARGET_ARM64)
#include "hwintrinsiclistarm64.h"
#else
#error Unsupported platform
#endif

#endif // FEATURE_HW_INTRINSICS

/* static */ constexpr uint8_t ValueNumStore::GetOpAttribsForArity(genTreeOps oper, GenTreeOperKind kind)
{
return ((GenTree::StaticOperIs(oper, GT_SELECT) ? 3 : (((kind & GTK_UNOP) >> 1) | ((kind & GTK_BINOP) >> 1)))
Expand Down Expand Up @@ -10434,8 +10394,8 @@ const uint8_t ValueNumStore::s_vnfOpAttribs[VNF_COUNT] = {

0, // VNF_Boundary

#define ValueNumFuncDef(vnf, arity, commute, knownNonNull, sharedStatic, extra) \
GetOpAttribsForFunc((arity) + static_cast<int>(extra), commute, knownNonNull, sharedStatic),
#define ValueNumFuncDef(vnf, arity, commute, knownNonNull, sharedStatic) \
GetOpAttribsForFunc(arity, commute, knownNonNull, sharedStatic),
#include "valuenumfuncs.h"
};

Expand Down Expand Up @@ -10490,7 +10450,7 @@ void ValueNumStore::ValidateValueNumStoreStatics()

int vnfNum = VNF_Boundary + 1; // The macro definition below will update this after using it.

#define ValueNumFuncDef(vnf, arity, commute, knownNonNull, sharedStatic, extra) \
#define ValueNumFuncDef(vnf, arity, commute, knownNonNull, sharedStatic) \
if (commute) \
arr[vnfNum] |= VNFOA_Commutative; \
if (knownNonNull) \
Expand All @@ -10514,19 +10474,6 @@ void ValueNumStore::ValidateValueNumStoreStatics()
for (NamedIntrinsic id = (NamedIntrinsic)(NI_HW_INTRINSIC_START + 1); (id < NI_HW_INTRINSIC_END);
id = (NamedIntrinsic)(id + 1))
{
bool encodeResultType = Compiler::vnEncodesResultTypeForHWIntrinsic(id);

if (encodeResultType)
{
// These HW_Intrinsic's have an extra VNF_SimdType arg.
//
VNFunc func = VNFunc(VNF_HWI_FIRST + (id - NI_HW_INTRINSIC_START - 1));
unsigned oldArity = (arr[func] & VNFOA_ArityMask) >> VNFOA_ArityShift;
unsigned newArity = oldArity + 1;

ValueNumFuncSetArity(func, newArity);
}

if (HWIntrinsicInfo::IsCommutative(id))
{
VNFunc func = VNFunc(VNF_HWI_FIRST + (id - NI_HW_INTRINSIC_START - 1));
Expand All @@ -10553,7 +10500,7 @@ void ValueNumStore::ValidateValueNumStoreStatics()

#ifdef DEBUG
// Define the name array.
#define ValueNumFuncDef(vnf, arity, commute, knownNonNull, sharedStatic, extra) #vnf,
#define ValueNumFuncDef(vnf, arity, commute, knownNonNull, sharedStatic) #vnf,

const char* ValueNumStore::VNFuncNameArr[] = {
#include "valuenumfuncs.h"
Expand Down Expand Up @@ -13014,19 +12961,13 @@ void Compiler::fgValueNumberHWIntrinsic(GenTreeHWIntrinsic* tree)
}
else
{
VNFunc func = GetVNFuncForNode(tree);
ValueNumPair resultTypeVNPair = ValueNumPair();
bool encodeResultType = vnEncodesResultTypeForHWIntrinsic(intrinsicId);
VNFunc func = GetVNFuncForNode(tree);
ValueNum simdTypeVN = vnStore->VNForSimdType(tree->GetSimdSize(), tree->GetNormalizedSimdBaseJitType());
ValueNumPair resultTypeVNPair(simdTypeVN, simdTypeVN);

if (encodeResultType)
{
ValueNum simdTypeVN = vnStore->VNForSimdType(tree->GetSimdSize(), tree->GetNormalizedSimdBaseJitType());
resultTypeVNPair.SetBoth(simdTypeVN);

JITDUMP(" simdTypeVN is ");
JITDUMPEXEC(vnPrint(simdTypeVN, 1));
JITDUMP("\n");
}
JITDUMP(" simdTypeVN is ");
JITDUMPEXEC(vnPrint(simdTypeVN, 1));
JITDUMP("\n");

auto getOperandVNs = [this, addr](GenTree* operand, ValueNumPair* pNormVNPair, ValueNumPair* pExcVNPair) {
vnStore->VNPUnpackExc(operand->gtVNPair, pNormVNPair, pExcVNPair);
Expand All @@ -13049,22 +12990,12 @@ void Compiler::fgValueNumberHWIntrinsic(GenTreeHWIntrinsic* tree)
}
};

const bool isVariableNumArgs = HWIntrinsicInfo::lookupNumArgs(intrinsicId) == -1;

// There are some HWINTRINSICS operations that have zero args, i.e. NI_Vector128_Zero
if (opCount == 0)
{
if (encodeResultType)
{
// There are zero arg HWINTRINSICS operations that encode the result type, i.e. Vector128_AllBitSet
normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, resultTypeVNPair);
assert(vnStore->VNFuncArity(func) == 1);
}
else
{
normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func);
assert(vnStore->VNFuncArity(func) == 0);
}
// There are zero arg HWINTRINSICS operations that encode the result type, i.e. Vector128_AllBitSet
normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, resultTypeVNPair);
assert(vnStore->VNFuncArity(func) == 1);
}
else // HWINTRINSIC unary or binary or ternary operator.
{
Expand All @@ -13074,11 +13005,10 @@ void Compiler::fgValueNumberHWIntrinsic(GenTreeHWIntrinsic* tree)

if (opCount == 1)
{
ValueNum normalLVN = vnStore->EvalHWIntrinsicFunUnary(tree, func, op1vnp.GetLiberal(), encodeResultType,
resultTypeVNPair.GetLiberal());
ValueNum normalCVN =
vnStore->EvalHWIntrinsicFunUnary(tree, func, op1vnp.GetConservative(), encodeResultType,
resultTypeVNPair.GetConservative());
ValueNum normalLVN =
vnStore->EvalHWIntrinsicFunUnary(tree, func, op1vnp.GetLiberal(), resultTypeVNPair.GetLiberal());
ValueNum normalCVN = vnStore->EvalHWIntrinsicFunUnary(tree, func, op1vnp.GetConservative(),
resultTypeVNPair.GetConservative());

normalPair = ValueNumPair(normalLVN, normalCVN);
excSetPair = op1Xvnp;
Expand All @@ -13093,10 +13023,10 @@ void Compiler::fgValueNumberHWIntrinsic(GenTreeHWIntrinsic* tree)
{
ValueNum normalLVN =
vnStore->EvalHWIntrinsicFunBinary(tree, func, op1vnp.GetLiberal(), op2vnp.GetLiberal(),
encodeResultType, resultTypeVNPair.GetLiberal());
ValueNum normalCVN = vnStore->EvalHWIntrinsicFunBinary(tree, func, op1vnp.GetConservative(),
op2vnp.GetConservative(), encodeResultType,
resultTypeVNPair.GetConservative());
resultTypeVNPair.GetLiberal());
ValueNum normalCVN =
vnStore->EvalHWIntrinsicFunBinary(tree, func, op1vnp.GetConservative(),
op2vnp.GetConservative(), resultTypeVNPair.GetConservative());

normalPair = ValueNumPair(normalLVN, normalCVN);
excSetPair = vnStore->VNPExcSetUnion(op1Xvnp, op2Xvnp);
Expand All @@ -13111,12 +13041,11 @@ void Compiler::fgValueNumberHWIntrinsic(GenTreeHWIntrinsic* tree)

ValueNum normalLVN =
vnStore->EvalHWIntrinsicFunTernary(tree, func, op1vnp.GetLiberal(), op2vnp.GetLiberal(),
op3vnp.GetLiberal(), encodeResultType,
resultTypeVNPair.GetLiberal());
op3vnp.GetLiberal(), resultTypeVNPair.GetLiberal());
ValueNum normalCVN =
vnStore->EvalHWIntrinsicFunTernary(tree, func, op1vnp.GetConservative(),
op2vnp.GetConservative(), op3vnp.GetConservative(),
encodeResultType, resultTypeVNPair.GetConservative());
resultTypeVNPair.GetConservative());

normalPair = ValueNumPair(normalLVN, normalCVN);

Expand Down
Loading

0 comments on commit 1524546

Please sign in to comment.