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

Add ValueNumbering support for GT_SIMD and GT_HWINTRINSIC tree nodes #31834

Merged
merged 34 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
0c4b857
Added ValueNumbering support for GT_SIMD and GT_HWINTRINSIC tree nodes
briansull Jan 30, 2020
367da1f
Allow SIMD and HW Intrinsics to be CSE candidates
briansull Jan 31, 2020
57eadfb
Correctness fix for optAssertionPropMain
briansull Feb 4, 2020
6d2028c
Improve the VNFOA_ArityMask
briansull Feb 4, 2020
d8b9b53
Include node type when value numbering SIMDIntrinsicInit
briansull Feb 6, 2020
e188b9b
Disable CSE's for some special HW_INTRINSIC categories
briansull Feb 7, 2020
506a544
Code review feedback
briansull Feb 8, 2020
9c62167
Record csdStructHnd; // The class handle, currently needed to create …
briansull Feb 12, 2020
8e3cea8
Fix the JITDUMP messages to print the CseIndex
briansull Feb 12, 2020
3769197
add check for (newElemStructHnd != NO_CLASS_HANDLE)
briansull Feb 12, 2020
305a017
Fix the printing of BitSets on Linux, change the printf format specifier
briansull Feb 14, 2020
0d0e4f8
Added check for simdNode->OperIsMemoryLoad()) to fgValueNumberSimd
briansull Feb 5, 2020
0232044
Update to use the new TARGET macros
briansull Feb 6, 2020
d9dda0a
Avoid calling gtGetStructHandleIfPresent to set csdStructHnd when we …
briansull Feb 12, 2020
f6d7f65
Instead of asserting on a struct handle mismatch, we record it in csd…
briansull Feb 12, 2020
b9d0a58
Fix check for (newElemStructHnd != hashDsc->csdStructHnd)
briansull Feb 12, 2020
3c3f2fe
Additional checks for SIMD struct types when setting csdStructHnd
briansull Feb 13, 2020
5b641a2
added extra value number argument VNF_SimdType for Most SIMD operations
briansull Feb 14, 2020
a8ef319
fix GenTreeSIMD::OperIsMemoryLoad for ARM64
briansull Feb 14, 2020
343e35d
Added bool methods vnEncodesResultTypeForSIMDIntrinsic and vnEncodesR…
briansull Feb 20, 2020
3c641ab
Fix for SIMD_WidenLo arg count
briansull Feb 20, 2020
c1aa65a
fix typo
briansull Feb 20, 2020
55228a6
Fix x86 build breaks
briansull Feb 20, 2020
2bbd982
Added method header comment for vnEncodesResultTypeForHWIntrinsic
briansull Feb 21, 2020
a0e6a8c
Codereview feedback and some more comments
briansull Feb 21, 2020
cbe5538
fix typo
briansull Feb 21, 2020
3e927bf
Moved the code that sets the arg count for the three SIMD intrinsics
briansull Feb 21, 2020
b2b8986
clang-format
briansull Feb 22, 2020
7bd9303
Adjust CSE for SIMD types that are live across a call
briansull Feb 25, 2020
169c24e
Proposed fix for #32085
briansull Feb 25, 2020
8d0ce82
Revert "Proposed fix for #32085"
briansull Feb 25, 2020
6ae894a
Added better comments for optcse SIMD caller saved register heuristics
briansull Feb 25, 2020
b4eb406
Added CONFIG_INTEGER: JitDisableSimdVN,
briansull Feb 26, 2020
90bf7b3
Moved JitDisableSimdVN from DEBUG to RETAIL
briansull Feb 26, 2020
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
1 change: 1 addition & 0 deletions src/coreclr/src/jit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ if (CLR_CMAKE_TARGET_WIN32)
unwind.h
utils.h
valuenum.h
valuenumfuncs.h
valuenumtype.h
varset.h
vartype.h
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/src/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5193,8 +5193,15 @@ void Compiler::optAssertionPropMain()
}
}

if (!optAssertionCount)
if (optAssertionCount == 0)
{
// Zero out the bbAssertionIn values, as these can be referenced in RangeCheck::MergeAssertion
// and this is sharedstate with the CSE phase: bbCseIn
//
for (BasicBlock* block = fgFirstBB; block; block = block->bbNext)
{
block->bbAssertionIn = BitVecOps::MakeEmpty(apTraits);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug fix that caused an A/V when the number of assertions was more than 64 and we had left over bits set from running the CSE phase. (This field is a union with bbCseIn)

    union {
        EXPSET_TP bbCseIn; // CSEs available on entry
#if ASSERTION_PROP
        ASSERT_TP bbAssertionIn; // value assignments available on entry
#endif
    };

return;
}

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/src/jit/bitsetasshortlong.h
Original file line number Diff line number Diff line change
Expand Up @@ -412,12 +412,12 @@ class BitSetOps</*BitSetType*/ BitSetShortLongRep,
char* ptr = res;
if (sizeof(size_t) == sizeof(int64_t))
{
sprintf_s(ptr, remaining, "%016zX", bits);
sprintf_s(ptr, remaining, "%016llX", bits);
}
else
{
assert(sizeof(size_t) == sizeof(int));
sprintf_s(ptr, remaining, "%08zX", bits);
sprintf_s(ptr, remaining, "%08X", bits);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the JitDump on Ubuntu Linux system which return -1 when they see a "z" value in the format string

}
return res;
}
Expand Down
26 changes: 26 additions & 0 deletions src/coreclr/src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4645,6 +4645,16 @@ class Compiler
// Does value-numbering for an intrinsic tree.
void fgValueNumberIntrinsic(GenTree* tree);

#ifdef FEATURE_SIMD
// Does value-numbering for a GT_SIMD tree
void fgValueNumberSimd(GenTree* tree);
#endif // FEATURE_SIMD

#ifdef FEATURE_HW_INTRINSICS
// Does value-numbering for a GT_HWINTRINSIC tree
void fgValueNumberHWIntrinsic(GenTree* tree);
#endif // FEATURE_HW_INTRINSICS

// Does value-numbering for a call. We interpret some helper calls.
void fgValueNumberCall(GenTreeCall* call);

Expand All @@ -4669,6 +4679,9 @@ class Compiler
// Adds the exception set for the current tree node which is performing a overflow checking operation
void fgValueNumberAddExceptionSetForOverflow(GenTree* tree);

// Adds the exception set for the current tree node which is performing a bounds check operation
void fgValueNumberAddExceptionSetForBoundsCheck(GenTree* tree);

// Adds the exception set for the current tree node which is performing a ckfinite operation
void fgValueNumberAddExceptionSetForCkFinite(GenTree* tree);

Expand Down Expand Up @@ -6163,6 +6176,12 @@ class Compiler
treeStmtLst* csdTreeList; // list of matching tree nodes: head
treeStmtLst* csdTreeLast; // list of matching tree nodes: tail

// ToDo: This can be removed when gtGetStructHandleIfPresent stops guessing
// and GT_IND nodes always have valid struct handle.
//
CORINFO_CLASS_HANDLE csdStructHnd; // The class handle, currently needed to create a SIMD LclVar in PerformCSE
bool csdStructHndMismatch;

ValueNum defExcSetPromise; // The exception set that is now required for all defs of this CSE.
// This will be set to NoVN if we decide to abandon this CSE

Expand Down Expand Up @@ -8162,6 +8181,13 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#endif // !FEATURE_SIMD
}

#ifdef FEATURE_SIMD
static bool vnEncodesResultTypeForSIMDIntrinsic(SIMDIntrinsicID intrinsicId);
#endif // !FEATURE_SIMD
#ifdef FEATURE_HW_INTRINSICS
static bool vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID);
#endif // FEATURE_HW_INTRINSICS

private:
// These routines need not be enclosed under FEATURE_SIMD since lvIsSIMDType()
// is defined for both FEATURE_SIMD and !FEATURE_SIMD apropriately. The use
Expand Down
25 changes: 20 additions & 5 deletions src/coreclr/src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5398,11 +5398,16 @@ bool GenTree::OperIsImplicitIndir() const
case GT_ARR_ELEM:
case GT_ARR_OFFSET:
return true;
#ifdef FEATURE_SIMD
case GT_SIMD:
{
return AsSIMD()->OperIsMemoryLoad();
}
#endif // FEATURE_SIMD
#ifdef FEATURE_HW_INTRINSICS
case GT_HWINTRINSIC:
briansull marked this conversation as resolved.
Show resolved Hide resolved
{
GenTreeHWIntrinsic* hwIntrinsicNode = (const_cast<GenTree*>(this))->AsHWIntrinsic();
return hwIntrinsicNode->OperIsMemoryLoadOrStore();
return AsHWIntrinsic()->OperIsMemoryLoadOrStore();
}
#endif // FEATURE_HW_INTRINSICS
default:
Expand Down Expand Up @@ -18118,6 +18123,16 @@ bool GenTree::isCommutativeSIMDIntrinsic()
return false;
}
}

// Returns true for the SIMD Instrinsic instructions that have MemoryLoad semantics, false otherwise
bool GenTreeSIMD::OperIsMemoryLoad() const
{
if (gtSIMDIntrinsicID == SIMDIntrinsicInitArray)
{
return true;
}
return false;
}
#endif // FEATURE_SIMD

#ifdef FEATURE_HW_INTRINSICS
Expand Down Expand Up @@ -18327,7 +18342,7 @@ GenTree* Compiler::gtNewMustThrowException(unsigned helper, var_types type, CORI
}

// Returns true for the HW Instrinsic instructions that have MemoryLoad semantics, false otherwise
bool GenTreeHWIntrinsic::OperIsMemoryLoad()
bool GenTreeHWIntrinsic::OperIsMemoryLoad() const
{
#ifdef TARGET_XARCH
// Some xarch instructions have MemoryLoad sematics
Expand Down Expand Up @@ -18369,7 +18384,7 @@ bool GenTreeHWIntrinsic::OperIsMemoryLoad()
}

// Returns true for the HW Instrinsic instructions that have MemoryStore semantics, false otherwise
bool GenTreeHWIntrinsic::OperIsMemoryStore()
bool GenTreeHWIntrinsic::OperIsMemoryStore() const
{
#ifdef TARGET_XARCH
// Some xarch instructions have MemoryStore sematics
Expand Down Expand Up @@ -18404,7 +18419,7 @@ bool GenTreeHWIntrinsic::OperIsMemoryStore()
}

// Returns true for the HW Instrinsic instructions that have MemoryLoad semantics, false otherwise
bool GenTreeHWIntrinsic::OperIsMemoryLoadOrStore()
bool GenTreeHWIntrinsic::OperIsMemoryLoadOrStore() const
{
#ifdef TARGET_XARCH
return OperIsMemoryLoad() || OperIsMemoryStore();
Expand Down
11 changes: 7 additions & 4 deletions src/coreclr/src/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -4536,6 +4536,9 @@ struct GenTreeSIMD : public GenTreeJitIntrinsic
{
}

bool OperIsMemoryLoad() const; // Returns true for the SIMD Instrinsic instructions that have MemoryLoad semantics,
// false otherwise

#if DEBUGGABLE_GENTREE
GenTreeSIMD() : GenTreeJitIntrinsic()
{
Expand Down Expand Up @@ -4584,12 +4587,12 @@ struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic
// However there are HW Instrinsic instructions that have 3 or even 4 operands and this is
// supported using a single op1 and using an ArgList for it: gtNewArgList(op1, op2, op3)

bool OperIsMemoryLoad(); // Returns true for the HW Instrinsic instructions that have MemoryLoad semantics,
bool OperIsMemoryLoad() const; // Returns true for the HW Instrinsic instructions that have MemoryLoad semantics,
// false otherwise
bool OperIsMemoryStore(); // Returns true for the HW Instrinsic instructions that have MemoryStore semantics,
bool OperIsMemoryStore() const; // Returns true for the HW Instrinsic instructions that have MemoryStore semantics,
// false otherwise
bool OperIsMemoryLoadOrStore(); // Returns true for the HW Instrinsic instructions that have MemoryLoad or
// MemoryStore semantics, false otherwise
bool OperIsMemoryLoadOrStore() const; // Returns true for the HW Instrinsic instructions that have MemoryLoad or
// MemoryStore semantics, false otherwise

#if DEBUGGABLE_GENTREE
GenTreeHWIntrinsic() : GenTreeJitIntrinsic()
Expand Down
63 changes: 63 additions & 0 deletions src/coreclr/src/jit/hwintrinsic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,69 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleForHWSIMD(var_types simdType, va
return NO_CLASS_HANDLE;
}

#ifdef FEATURE_HW_INTRINSICS
//------------------------------------------------------------------------
// 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 subsitutions.
//
/* static */ bool Compiler::vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID)
briansull marked this conversation as resolved.
Show resolved Hide resolved
{
int numArgs = HWIntrinsicInfo::lookupNumArgs(hwIntrinsicID);

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

// We iterate over all of the different baseType's for this instrinsic 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);
}
#endif // FEATURE_HW_INTRINSICS

//------------------------------------------------------------------------
// lookupId: Gets the NamedIntrinsic for a given method name and InstructionSet
//
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/src/jit/hwintrinsiclistxarch.h
Original file line number Diff line number Diff line change
Expand Up @@ -483,10 +483,10 @@ HARDWARE_INTRINSIC(AVX2_ConvertToUInt32, "ConvertToUI
HARDWARE_INTRINSIC(AVX2_ConvertToVector256Int16, "ConvertToVector256Int16", AVX2, -1, 32, 1, {INS_pmovsxbw, INS_pmovzxbw, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_SpecialCodeGen|HW_Flag_BaseTypeFromFirstArg)
HARDWARE_INTRINSIC(AVX2_ConvertToVector256Int32, "ConvertToVector256Int32", AVX2, -1, 32, 1, {INS_pmovsxbd, INS_pmovzxbd, INS_pmovsxwd, INS_pmovzxwd, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_SpecialCodeGen|HW_Flag_BaseTypeFromFirstArg)
HARDWARE_INTRINSIC(AVX2_ConvertToVector256Int64, "ConvertToVector256Int64", AVX2, -1, 32, 1, {INS_pmovsxbq, INS_pmovzxbq, INS_pmovsxwq, INS_pmovzxwq, INS_pmovsxdq, INS_pmovzxdq, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_SpecialCodeGen|HW_Flag_BaseTypeFromFirstArg)
HARDWARE_INTRINSIC(AVX2_GatherVector128, "GatherVector128", AVX2, -1, 16, 3, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_GatherVector256, "GatherVector256", AVX2, -1, 32, 3, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_GatherMaskVector128, "GatherMaskVector128", AVX2, -1, 16, 5, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_GatherMaskVector256, "GatherMaskVector256", AVX2, -1, 32, 5, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_GatherVector128, "GatherVector128", AVX2, -1, 16, 3, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_GatherVector256, "GatherVector256", AVX2, -1, 32, 3, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_GatherMaskVector128, "GatherMaskVector128", AVX2, -1, 16, 5, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_GatherMaskVector256, "GatherMaskVector256", AVX2, -1, 32, 5, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpgatherdd, INS_vpgatherdd, INS_vpgatherdq, INS_vpgatherdq, INS_vgatherdps, INS_vgatherdpd}, HW_Category_IMM, HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
HARDWARE_INTRINSIC(AVX2_HorizontalAdd, "HorizontalAdd", AVX2, -1, 32, 2, {INS_invalid, INS_invalid, INS_phaddw, INS_invalid, INS_phaddd, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_NoFlag)
HARDWARE_INTRINSIC(AVX2_HorizontalAddSaturate, "HorizontalAddSaturate", AVX2, -1, 32, 2, {INS_invalid, INS_invalid, INS_phaddsw, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_NoFlag)
HARDWARE_INTRINSIC(AVX2_HorizontalSubtract, "HorizontalSubtract", AVX2, -1, 32, 2, {INS_invalid, INS_invalid, INS_phsubw, INS_invalid, INS_phsubd, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_NoFlag)
Expand Down
Loading