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.

Sign up for GitHub

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

Jump to bottom

Mark SIMD assignments as related to SIMD intrinsics #51731

Merged
merged 5 commits into from
Apr 27, 2021
Merged

Mark SIMD assignments as related to SIMD intrinsics #51731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e12823
Ensure that GenTreeJitIntrinsic explicitly zeroes out certain nodes
tannergooding Apr 23, 2021
0a50867
Mark SIMD assignments as related to SIMD intrinsics
tannergooding Apr 23, 2021
9debe4a
Refactor SetOpLclRelatedToSIMDIntrinsic so it is easier to debug/unde…
tannergooding Apr 23, 2021
954782a
Applying formatting patch
tannergooding Apr 23, 2021
a4c5ca3
Mark SIMD assignments as related to SIMD intrinsics on x86
tannergooding Apr 26, 2021
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
2 changes: 1 addition & 1 deletion src/coreclr/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1980,7 +1980,7 @@ void CodeGen::genMultiRegStoreToSIMDLocal(GenTreeLclVar* lclNode)
}
genProduceReg(lclNode);
#else // !UNIX_AMD64_ABI
assert(!"Multireg store to SIMD reg not supported on X64 Windows");
assert(!"Multireg store to SIMD reg not supported on Windows");
#endif // !UNIX_AMD64_ABI
}
#endif // FEATURE_SIMD
Expand Down
41 changes: 33 additions & 8 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6759,6 +6759,20 @@ GenTreeOp* Compiler::gtNewAssignNode(GenTree* dst, GenTree* src)
}
dst->gtFlags |= GTF_DONT_CSE;

#if defined(FEATURE_SIMD) && !defined(TARGET_X86)
// TODO-CQ: x86 Windows supports multi-reg returns but not SIMD multi-reg returns

if (varTypeIsSIMD(dst->gtType))
{
// We want to track SIMD assignments as being intrinsics since they
// are functionally SIMD `mov` instructions and are more efficient
// when we don't promote, particularly when it occurs due to inlining

SetOpLclRelatedToSIMDIntrinsic(dst);
SetOpLclRelatedToSIMDIntrinsic(src);
}
#endif // FEATURE_SIMD

/* Create the assignment node */

GenTreeOp* asg = gtNewOperNode(GT_ASG, dst->TypeGet(), dst, src)->AsOp();
Expand Down Expand Up @@ -18937,16 +18951,27 @@ GenTreeSIMD* Compiler::gtNewSIMDNode(var_types type,
//
void Compiler::SetOpLclRelatedToSIMDIntrinsic(GenTree* op)
{
if (op != nullptr)
if (op == nullptr)
{
if (op->OperIsLocal())
{
setLclRelatedToSIMDIntrinsic(op);
}
else if ((op->OperGet() == GT_OBJ) && (op->AsOp()->gtOp1->OperGet() == GT_ADDR) &&
op->AsOp()->gtOp1->AsOp()->gtOp1->OperIsLocal())
return;
}

if (op->OperIsLocal())
{
setLclRelatedToSIMDIntrinsic(op);
}
else if (op->OperIs(GT_OBJ))
{
GenTree* addr = op->AsIndir()->Addr();

if (addr->OperIs(GT_ADDR))
{
setLclRelatedToSIMDIntrinsic(op->AsOp()->gtOp1->AsOp()->gtOp1);
GenTree* addrOp1 = addr->AsOp()->gtGetOp1();

if (addrOp1->OperIsLocal())
{
setLclRelatedToSIMDIntrinsic(addrOp1);
}
Comment on lines +18963 to +18974
Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't clear why we were only checking for OBJ(ADDR(LCL)) here. We also sometimes generate BLK(ADDR(LCL)) or other nodes and so something like the following seems like it might be a better choice, given that it will cover all indirections over locals:

else if (op->OperIsIndir())
{
    GenTree* lcl = op->AsInidr()->Addr()->IsLocalAddrExpr();

    if (lcl != null)
    {
        setLclRelatedToSIMDIntrinsic(lcl);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't seem to be very consistent on what we check for calling setLclRelatedToSIMDIntrinsic either. The paths that create SIMD or HWINTRINSIC nodes all call SetOpLclRelatedToSIMDIntrinsic, while several other paths call setLclRelatedToSIMDIntrinsic directly.

Of those that call setLclRelatedToSIMDIntrinsic directly, they vary on what they check before calling setLclRelatedToSIMDIntrinsic. Sometimes simply checking OperIsLocal, sometimes checking for specific kinds of indirections, and sometimes checking all indirections. It would be nice (assuming nothing is blocking it), if all of those checks could centrally be handled here so they are consistent.

}
}
}
Expand Down
9 changes: 6 additions & 3 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4847,7 +4847,7 @@ struct GenTreeIntrinsic : public GenTreeOp
struct GenTreeJitIntrinsic : public GenTreeOp
{
private:
ClassLayout* m_layout;
ClassLayout* gtLayout;

unsigned char gtAuxiliaryJitType; // For intrinsics than need another type (e.g. Avx2.Gather* or SIMD (by element))
regNumberSmall gtOtherReg; // For intrinsics that return 2 registers
Expand All @@ -4867,13 +4867,13 @@ struct GenTreeJitIntrinsic : public GenTreeOp

ClassLayout* GetLayout() const
{
return m_layout;
return gtLayout;
}

void SetLayout(ClassLayout* layout)
{
assert(layout != nullptr);
m_layout = layout;
gtLayout = layout;
}

regNumber GetOtherReg() const
Expand Down Expand Up @@ -4927,6 +4927,9 @@ struct GenTreeJitIntrinsic : public GenTreeOp
GenTreeJitIntrinsic(
genTreeOps oper, var_types type, GenTree* op1, GenTree* op2, CorInfoType simdBaseJitType, unsigned simdSize)
: GenTreeOp(oper, type, op1, op2)
, gtLayout(nullptr)
, gtAuxiliaryJitType(CORINFO_TYPE_UNDEF)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed due to an assert encountered while running PMI diffs, its related to the refactoring that happened in #50832.
If these aren't explicitly zeroed, they may be 0xDD in debug builds and cause assertions when calling the helper functions used in hashing the trees.

, gtOtherReg(REG_NA)
, gtSimdBaseJitType((unsigned char)simdBaseJitType)
, gtSimdSize((unsigned char)simdSize)
, gtHWIntrinsicId(NI_Illegal)
Expand Down