Skip to content

Commit d7a30ba

Browse files
tannergoodingtannergooding
authored
Ensure Vector2/3/4, Quaternion, and Plane don't have a false dependency on Vector<T> (#86481)
* Ensure Vector2/3/4, Quaternion, and Plane don't have a false dependency on Vector<T> * Apply JIT formatting patch * Fixing a build issue * Handle an SPMI assert
1 parent fdb5ac7 commit d7a30ba

File tree

10 files changed

+159
-79
lines changed

10 files changed

+159
-79
lines changed

‎docs/design/coreclr/botr/vectors-and-intrinsics.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,5 +169,5 @@ While the above api exists, it is not expected that general purpose code within
169169
|`compExactlyDependsOn(isa)`| Use when making a decision to use or not use an instruction set when the decision will affect the semantics of the generated code. Should never be used in an assert. | Return whether or not an instruction set is supported. Calls notifyInstructionSetUsage with the result of that computation.
170170
|`compOpportunisticallyDependsOn(isa)`| Use when making an opportunistic decision to use or not use an instruction set. Use when the instruction set usage is a "nice to have optimization opportunity", but do not use when a false result may change the semantics of the program. Should never be used in an assert. | Return whether or not an instruction set is supported. Calls notifyInstructionSetUsage if the instruction set is supported.
171171
|`compIsaSupportedDebugOnly(isa)` | Use to assert whether or not an instruction set is supported | Return whether or not an instruction set is supported. Does not report anything. Only available in debug builds.
172-
|`getSIMDVectorRegisterByteLength()` | Use to get the size of a `Vector<T>` value. | Determine the size of the `Vector<T>` type. If on the architecture the size may vary depending on whatever rules. Use `compExactlyDependsOn` to perform the queries so that the size is consistent between compile time and runtime.
172+
|`getVectorTByteLength()` | Use to get the size of a `Vector<T>` value. | Determine the size of the `Vector<T>` type. If on the architecture the size may vary depending on whatever rules. Use `compExactlyDependsOn` to perform the queries so that the size is consistent between compile time and runtime.
173173
|`getMaxVectorByteLength()`| Get the maximum number of bytes that might be used in a SIMD type during this compilation. | Query the set of instruction sets supported, and determine the largest simd type supported. Use `compOpportunisticallyDependsOn` to perform the queries so that the maximum size needed is the only one recorded.

‎src/coreclr/jit/compiler.h

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8645,8 +8645,13 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
86458645

86468646
// Get the number of bytes in a System.Numeric.Vector<T> for the current compilation.
86478647
// Note - cannot be used for System.Runtime.Intrinsic
8648-
unsigned getSIMDVectorRegisterByteLength()
8648+
unsigned getVectorTByteLength()
86498649
{
8650+
// We need to report the ISA dependency to the VM so that scenarios
8651+
// such as R2R work correctly for larger vector sizes, so we always
8652+
// do `compExactlyDependsOn` for such cases.
8653+
CLANG_FORMAT_COMMENT_ANCHOR;
8654+
86508655
#if defined(TARGET_XARCH)
86518656
if (compExactlyDependsOn(InstructionSet_AVX2))
86528657
{
@@ -8660,7 +8665,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
86608665
#elif defined(TARGET_ARM64)
86618666
return FP_REGSIZE_BYTES;
86628667
#else
8663-
assert(!"getSIMDVectorRegisterByteLength() unimplemented on target arch");
8668+
assert(!"getVectorTByteLength() unimplemented on target arch");
86648669
unreached();
86658670
#endif
86668671
}

‎src/coreclr/jit/hwintrinsicxarch.cpp

Lines changed: 50 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -775,24 +775,26 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
775775
case NI_Vector128_AsVector:
776776
{
777777
assert(sig->numArgs == 1);
778+
uint32_t vectorTByteLength = getVectorTByteLength();
778779

779-
if (getSIMDVectorRegisterByteLength() == YMM_REGSIZE_BYTES)
780+
if (vectorTByteLength == YMM_REGSIZE_BYTES)
780781
{
781782
// Vector<T> is TYP_SIMD32, so we should treat this as a call to Vector128.ToVector256
782783
return impSpecialIntrinsic(NI_Vector128_ToVector256, clsHnd, method, sig, simdBaseJitType, retType,
783784
simdSize);
784785
}
786+
else
787+
{
788+
assert(vectorTByteLength == XMM_REGSIZE_BYTES);
785789

786-
assert(getSIMDVectorRegisterByteLength() == XMM_REGSIZE_BYTES);
787-
788-
// We fold away the cast here, as it only exists to satisfy
789-
// the type system. It is safe to do this here since the retNode type
790-
// and the signature return type are both the same TYP_SIMD.
791-
792-
retNode = impSIMDPopStack();
793-
SetOpLclRelatedToSIMDIntrinsic(retNode);
794-
assert(retNode->gtType == getSIMDTypeForSize(getSIMDTypeSizeInBytes(sig->retTypeSigClass)));
790+
// We fold away the cast here, as it only exists to satisfy
791+
// the type system. It is safe to do this here since the retNode type
792+
// and the signature return type are both the same TYP_SIMD.
795793

794+
retNode = impSIMDPopStack();
795+
SetOpLclRelatedToSIMDIntrinsic(retNode);
796+
assert(retNode->gtType == getSIMDTypeForSize(getSIMDTypeSizeInBytes(sig->retTypeSigClass)));
797+
}
796798
break;
797799
}
798800

@@ -903,8 +905,9 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
903905
case NI_Vector256_AsVector256:
904906
{
905907
assert(sig->numArgs == 1);
908+
uint32_t vectorTByteLength = getVectorTByteLength();
906909

907-
if (getSIMDVectorRegisterByteLength() == YMM_REGSIZE_BYTES)
910+
if (vectorTByteLength == YMM_REGSIZE_BYTES)
908911
{
909912
// We fold away the cast here, as it only exists to satisfy
910913
// the type system. It is safe to do this here since the retNode type
@@ -916,36 +919,38 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
916919

917920
break;
918921
}
919-
920-
assert(getSIMDVectorRegisterByteLength() == XMM_REGSIZE_BYTES);
921-
922-
if (compExactlyDependsOn(InstructionSet_AVX))
922+
else
923923
{
924-
// We support Vector256 but Vector<T> is only 16-bytes, so we should
925-
// treat this method as a call to Vector256.GetLower or Vector128.ToVector256
924+
assert(vectorTByteLength == XMM_REGSIZE_BYTES);
926925

927-
if (intrinsic == NI_Vector256_AsVector)
926+
if (compExactlyDependsOn(InstructionSet_AVX))
928927
{
929-
return impSpecialIntrinsic(NI_Vector256_GetLower, clsHnd, method, sig, simdBaseJitType, retType,
930-
simdSize);
931-
}
932-
else
933-
{
934-
assert(intrinsic == NI_Vector256_AsVector256);
935-
return impSpecialIntrinsic(NI_Vector128_ToVector256, clsHnd, method, sig, simdBaseJitType, retType,
936-
16);
928+
// We support Vector256 but Vector<T> is only 16-bytes, so we should
929+
// treat this method as a call to Vector256.GetLower or Vector128.ToVector256
930+
931+
if (intrinsic == NI_Vector256_AsVector)
932+
{
933+
return impSpecialIntrinsic(NI_Vector256_GetLower, clsHnd, method, sig, simdBaseJitType, retType,
934+
simdSize);
935+
}
936+
else
937+
{
938+
assert(intrinsic == NI_Vector256_AsVector256);
939+
return impSpecialIntrinsic(NI_Vector128_ToVector256, clsHnd, method, sig, simdBaseJitType,
940+
retType, 16);
941+
}
937942
}
938943
}
939-
940944
break;
941945
}
942946

943947
case NI_Vector512_AsVector:
944948
case NI_Vector512_AsVector512:
945949
{
946950
assert(sig->numArgs == 1);
951+
uint32_t vectorTByteLength = getVectorTByteLength();
947952

948-
if (getSIMDVectorRegisterByteLength() == YMM_REGSIZE_BYTES)
953+
if (vectorTByteLength == YMM_REGSIZE_BYTES)
949954
{
950955
assert(IsBaselineVector512IsaSupported());
951956
// We support Vector512 but Vector<T> is only 32-bytes, so we should
@@ -964,23 +969,26 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
964969
}
965970
break;
966971
}
967-
968-
assert(getSIMDVectorRegisterByteLength() == XMM_REGSIZE_BYTES);
969-
if (compExactlyDependsOn(InstructionSet_AVX512F))
972+
else
970973
{
971-
// We support Vector512 but Vector<T> is only 16-bytes, so we should
972-
// treat this method as a call to Vector512.GetLower128 or Vector128.ToVector512
974+
assert(vectorTByteLength == XMM_REGSIZE_BYTES);
973975

974-
if (intrinsic == NI_Vector512_AsVector)
976+
if (compExactlyDependsOn(InstructionSet_AVX512F))
975977
{
976-
return impSpecialIntrinsic(NI_Vector512_GetLower128, clsHnd, method, sig, simdBaseJitType, retType,
977-
simdSize);
978-
}
979-
else
980-
{
981-
assert(intrinsic == NI_Vector512_AsVector512);
982-
return impSpecialIntrinsic(NI_Vector128_ToVector512, clsHnd, method, sig, simdBaseJitType, retType,
983-
16);
978+
// We support Vector512 but Vector<T> is only 16-bytes, so we should
979+
// treat this method as a call to Vector512.GetLower128 or Vector128.ToVector512
980+
981+
if (intrinsic == NI_Vector512_AsVector)
982+
{
983+
return impSpecialIntrinsic(NI_Vector512_GetLower128, clsHnd, method, sig, simdBaseJitType,
984+
retType, simdSize);
985+
}
986+
else
987+
{
988+
assert(intrinsic == NI_Vector512_AsVector512);
989+
return impSpecialIntrinsic(NI_Vector128_ToVector512, clsHnd, method, sig, simdBaseJitType,
990+
retType, 16);
991+
}
984992
}
985993
}
986994
break;

‎src/coreclr/jit/importercalls.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8332,10 +8332,7 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
83328332
CORINFO_SIG_INFO sig;
83338333
info.compCompHnd->getMethodSig(method, &sig);
83348334

8335-
int sizeOfVectorT = getSIMDVectorRegisterByteLength();
8336-
8337-
result = SimdAsHWIntrinsicInfo::lookupId(this, &sig, className, methodName, enclosingClassName,
8338-
sizeOfVectorT);
8335+
result = SimdAsHWIntrinsicInfo::lookupId(this, &sig, className, methodName, enclosingClassName);
83398336
#endif // FEATURE_HW_INTRINSICS
83408337

83418338
if (result == NI_Illegal)

‎src/coreclr/jit/simd.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,6 @@ CorInfoType Compiler::getBaseJitTypeAndSizeOfSIMDType(CORINFO_CLASS_HANDLE typeH
233233
{
234234
JITDUMP(" Found type Vector\n");
235235
m_simdHandleCache->VectorHandle = typeHnd;
236-
237-
size = getSIMDVectorRegisterByteLength();
238236
break;
239237
}
240238

@@ -299,8 +297,9 @@ CorInfoType Compiler::getBaseJitTypeAndSizeOfSIMDType(CORINFO_CLASS_HANDLE typeH
299297
}
300298

301299
JITDUMP(" Found Vector<%s>\n", varTypeName(JitType2PreciseVarType(simdBaseJitType)));
300+
size = getVectorTByteLength();
302301

303-
size = getSIMDVectorRegisterByteLength();
302+
assert(size != 0);
304303
break;
305304
}
306305

‎src/coreclr/jit/simdashwintrinsic.cpp

Lines changed: 72 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -44,21 +44,21 @@ const SimdAsHWIntrinsicInfo& SimdAsHWIntrinsicInfo::lookup(NamedIntrinsic id)
4444
// lookupId: Gets the NamedIntrinsic for a given method name and InstructionSet
4545
//
4646
// Arguments:
47+
// comp -- The compiler
48+
// sig -- The signature of the intrinsic
4749
// className -- The name of the class associated with the SimdIntrinsic to lookup
4850
// methodName -- The name of the method associated with the SimdIntrinsic to lookup
4951
// enclosingClassName -- The name of the enclosing class
50-
// sizeOfVectorT -- The size of Vector<T> in bytes
5152
//
5253
// Return Value:
5354
// The NamedIntrinsic associated with methodName and classId
5455
NamedIntrinsic SimdAsHWIntrinsicInfo::lookupId(Compiler* comp,
5556
CORINFO_SIG_INFO* sig,
5657
const char* className,
5758
const char* methodName,
58-
const char* enclosingClassName,
59-
int sizeOfVectorT)
59+
const char* enclosingClassName)
6060
{
61-
SimdAsHWIntrinsicClassId classId = lookupClassId(className, enclosingClassName, sizeOfVectorT);
61+
SimdAsHWIntrinsicClassId classId = lookupClassId(comp, className, enclosingClassName);
6262

6363
if (classId == SimdAsHWIntrinsicClassId::Unknown)
6464
{
@@ -74,11 +74,46 @@ NamedIntrinsic SimdAsHWIntrinsicInfo::lookupId(Compiler* comp,
7474
isInstanceMethod = true;
7575
}
7676

77-
if (strcmp(methodName, "get_IsHardwareAccelerated") == 0)
77+
if (classId == SimdAsHWIntrinsicClassId::Vector)
7878
{
79-
return comp->IsBaselineSimdIsaSupported() ? NI_IsSupported_True : NI_IsSupported_False;
79+
// We want to avoid doing anything that would unnecessarily trigger a recorded dependency against Vector<T>
80+
// so we duplicate a few checks here to ensure this works smoothly for the static Vector class.
81+
82+
assert(!isInstanceMethod);
83+
84+
if (strcmp(methodName, "get_IsHardwareAccelerated") == 0)
85+
{
86+
return comp->IsBaselineSimdIsaSupported() ? NI_IsSupported_True : NI_IsSupported_False;
87+
}
88+
89+
var_types retType = JITtype2varType(sig->retType);
90+
CorInfoType simdBaseJitType = CORINFO_TYPE_UNDEF;
91+
CORINFO_CLASS_HANDLE argClass = NO_CLASS_HANDLE;
92+
93+
if (retType == TYP_STRUCT)
94+
{
95+
argClass = sig->retTypeSigClass;
96+
}
97+
else
98+
{
99+
assert(numArgs != 0);
100+
argClass = comp->info.compCompHnd->getArgClass(sig, sig->args);
101+
}
102+
103+
const char* argNamespaceName;
104+
const char* argClassName = comp->getClassNameFromMetadata(argClass, &argNamespaceName);
105+
106+
classId = lookupClassId(comp, argClassName, nullptr);
107+
108+
if (classId == SimdAsHWIntrinsicClassId::Unknown)
109+
{
110+
return NI_Illegal;
111+
}
112+
assert(classId != SimdAsHWIntrinsicClassId::Vector);
80113
}
81114

115+
assert(strcmp(methodName, "get_IsHardwareAccelerated") != 0);
116+
82117
for (int i = 0; i < (NI_SIMD_AS_HWINTRINSIC_END - NI_SIMD_AS_HWINTRINSIC_START - 1); i++)
83118
{
84119
const SimdAsHWIntrinsicInfo& intrinsicInfo = simdAsHWIntrinsicInfoArray[i];
@@ -113,19 +148,17 @@ NamedIntrinsic SimdAsHWIntrinsicInfo::lookupId(Compiler* comp,
113148
// lookupClassId: Gets the SimdAsHWIntrinsicClassId for a given class name and enclsoing class name
114149
//
115150
// Arguments:
151+
// comp -- The compiler
116152
// className -- The name of the class associated with the SimdAsHWIntrinsicClassId to lookup
117153
// enclosingClassName -- The name of the enclosing class
118-
// sizeOfVectorT -- The size of Vector<T> in bytes
119154
//
120155
// Return Value:
121156
// The SimdAsHWIntrinsicClassId associated with className and enclosingClassName
122-
SimdAsHWIntrinsicClassId SimdAsHWIntrinsicInfo::lookupClassId(const char* className,
123-
const char* enclosingClassName,
124-
int sizeOfVectorT)
157+
SimdAsHWIntrinsicClassId SimdAsHWIntrinsicInfo::lookupClassId(Compiler* comp,
158+
const char* className,
159+
const char* enclosingClassName)
125160
{
126-
assert(className != nullptr);
127-
128-
if (enclosingClassName != nullptr)
161+
if ((className == nullptr) || (enclosingClassName != nullptr))
129162
{
130163
return SimdAsHWIntrinsicClassId::Unknown;
131164
}
@@ -159,7 +192,11 @@ SimdAsHWIntrinsicClassId SimdAsHWIntrinsicInfo::lookupClassId(const char* classN
159192

160193
className += 6;
161194

162-
if (strcmp(className, "2") == 0)
195+
if (className[0] == '\0')
196+
{
197+
return SimdAsHWIntrinsicClassId::Vector;
198+
}
199+
else if (strcmp(className, "2") == 0)
163200
{
164201
return SimdAsHWIntrinsicClassId::Vector2;
165202
}
@@ -171,16 +208,18 @@ SimdAsHWIntrinsicClassId SimdAsHWIntrinsicInfo::lookupClassId(const char* classN
171208
{
172209
return SimdAsHWIntrinsicClassId::Vector4;
173210
}
174-
else if ((className[0] == '\0') || (strcmp(className, "`1") == 0))
211+
else if (strcmp(className, "`1") == 0)
175212
{
213+
uint32_t vectorTByteLength = comp->getVectorTByteLength();
214+
176215
#if defined(TARGET_XARCH)
177-
if (sizeOfVectorT == 32)
216+
if (vectorTByteLength == 32)
178217
{
179218
return SimdAsHWIntrinsicClassId::VectorT256;
180219
}
181220
#endif // TARGET_XARCH
182221

183-
assert(sizeOfVectorT == 16);
222+
assert(vectorTByteLength == 16);
184223
return SimdAsHWIntrinsicClassId::VectorT128;
185224
}
186225
break;
@@ -655,6 +694,10 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,
655694
break;
656695
}
657696

697+
case NI_Quaternion_WithElement:
698+
case NI_Vector2_WithElement:
699+
case NI_Vector3_WithElement:
700+
case NI_Vector4_WithElement:
658701
case NI_VectorT128_WithElement:
659702
case NI_VectorT256_WithElement:
660703
{
@@ -736,6 +779,10 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,
736779
break;
737780
}
738781

782+
case NI_Quaternion_WithElement:
783+
case NI_Vector2_WithElement:
784+
case NI_Vector3_WithElement:
785+
case NI_Vector4_WithElement:
739786
case NI_VectorT128_WithElement:
740787
{
741788
assert(numArgs == 3);
@@ -1484,9 +1531,13 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,
14841531
}
14851532

14861533
case NI_Quaternion_get_Item:
1534+
case NI_Quaternion_GetElement:
14871535
case NI_Vector2_get_Item:
1536+
case NI_Vector2_GetElement:
14881537
case NI_Vector3_get_Item:
1538+
case NI_Vector3_GetElement:
14891539
case NI_Vector4_get_Item:
1540+
case NI_Vector4_GetElement:
14901541
case NI_VectorT128_get_Item:
14911542
case NI_VectorT128_GetElement:
14921543
#if defined(TARGET_XARCH)
@@ -1986,6 +2037,10 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,
19862037
break;
19872038
}
19882039

2040+
case NI_Quaternion_WithElement:
2041+
case NI_Vector2_WithElement:
2042+
case NI_Vector3_WithElement:
2043+
case NI_Vector4_WithElement:
19892044
case NI_VectorT128_WithElement:
19902045
#if defined(TARGET_XARCH)
19912046
case NI_VectorT256_WithElement:

0 commit comments

Comments
 (0)