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

Improve struct inits. #52292

Merged
merged 2 commits into from
May 8, 2021
Merged

Improve struct inits. #52292

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb7356e
Support STORE_LCL_VAR(0) after lowering.
May 4, 2021
e8e2a8b
fix a nice catch from Tanner
May 7, 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
17 changes: 3 additions & 14 deletions src/coreclr/jit/codegenarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3846,20 +3846,9 @@ void CodeGen::genEmitHelperCall(unsigned helper, int argSize, emitAttr retSize,
void CodeGen::genSIMDIntrinsic(GenTreeSIMD* simdNode)
{
// NYI for unsupported base types
if (simdNode->GetSimdBaseType() != TYP_INT && simdNode->GetSimdBaseType() != TYP_LONG &&
simdNode->GetSimdBaseType() != TYP_FLOAT && simdNode->GetSimdBaseType() != TYP_DOUBLE &&
simdNode->GetSimdBaseType() != TYP_USHORT && simdNode->GetSimdBaseType() != TYP_UBYTE &&
simdNode->GetSimdBaseType() != TYP_SHORT && simdNode->GetSimdBaseType() != TYP_BYTE &&
simdNode->GetSimdBaseType() != TYP_UINT && simdNode->GetSimdBaseType() != TYP_ULONG)
{
// We don't need a base type for the Upper Save & Restore intrinsics, and we may find
// these implemented over lclVars created by CSE without full handle information (and
// therefore potentially without a base type).
if ((simdNode->gtSIMDIntrinsicID != SIMDIntrinsicUpperSave) &&
(simdNode->gtSIMDIntrinsicID != SIMDIntrinsicUpperRestore))
{
noway_assert(!"SIMD intrinsic with unsupported base type.");
}
if (!varTypeIsArithmetic(simdNode->GetSimdBaseType()))
{
noway_assert(!"SIMD intrinsic with unsupported base type.");
}

switch (simdNode->gtSIMDIntrinsicID)
Expand Down
30 changes: 15 additions & 15 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1688,10 +1688,10 @@ struct GenTree
bool IsValidCallArgument();
#endif // DEBUG

inline bool IsFPZero();
inline bool IsIntegralConst(ssize_t constVal);
inline bool IsIntegralConstVector(ssize_t constVal);
inline bool IsSIMDZero();
inline bool IsFPZero() const;
inline bool IsIntegralConst(ssize_t constVal) const;
inline bool IsIntegralConstVector(ssize_t constVal) const;
inline bool IsSIMDZero() const;

inline bool IsBoxedValue();

Expand Down Expand Up @@ -2922,11 +2922,11 @@ struct GenTreeVal : public GenTree

struct GenTreeIntConCommon : public GenTree
{
inline INT64 LngValue();
inline INT64 LngValue() const;
inline void SetLngValue(INT64 val);
inline ssize_t IconValue();
inline ssize_t IconValue() const;
inline void SetIconValue(ssize_t val);
inline INT64 IntegralValue();
inline INT64 IntegralValue() const;

GenTreeIntConCommon(genTreeOps oper, var_types type DEBUGARG(bool largeNode = false))
: GenTree(oper, type DEBUGARG(largeNode))
Expand Down Expand Up @@ -3090,7 +3090,7 @@ struct GenTreeLngCon : public GenTreeIntConCommon
#endif
};

inline INT64 GenTreeIntConCommon::LngValue()
inline INT64 GenTreeIntConCommon::LngValue() const
{
#ifndef TARGET_64BIT
assert(gtOper == GT_CNS_LNG);
Expand All @@ -3114,7 +3114,7 @@ inline void GenTreeIntConCommon::SetLngValue(INT64 val)
#endif
}

inline ssize_t GenTreeIntConCommon::IconValue()
inline ssize_t GenTreeIntConCommon::IconValue() const
{
assert(gtOper == GT_CNS_INT); // We should never see a GT_CNS_LNG for a 64-bit target!
return AsIntCon()->gtIconVal;
Expand All @@ -3126,7 +3126,7 @@ inline void GenTreeIntConCommon::SetIconValue(ssize_t val)
AsIntCon()->gtIconVal = val;
}

inline INT64 GenTreeIntConCommon::IntegralValue()
inline INT64 GenTreeIntConCommon::IntegralValue() const
{
#ifdef TARGET_64BIT
return LngValue();
Expand Down Expand Up @@ -6942,7 +6942,7 @@ inline bool GenTree::OperIsCopyBlkOp()
// Return Value:
// Returns true iff the tree is an GT_CNS_DBL, with value of 0.0.

inline bool GenTree::IsFPZero()
inline bool GenTree::IsFPZero() const
{
if ((gtOper == GT_CNS_DBL) && (AsDblCon()->gtDconVal == 0.0))
{
Expand All @@ -6965,7 +6965,7 @@ inline bool GenTree::IsFPZero()
// Like gtIconVal, the argument is of ssize_t, so cannot check for
// long constants in a target-independent way.

inline bool GenTree::IsIntegralConst(ssize_t constVal)
inline bool GenTree::IsIntegralConst(ssize_t constVal) const

{
if ((gtOper == GT_CNS_INT) && (AsIntConCommon()->IconValue() == constVal))
Expand All @@ -6991,7 +6991,7 @@ inline bool GenTree::IsIntegralConst(ssize_t constVal)
// Returns:
// True if this represents an integral const SIMD vector.
//
inline bool GenTree::IsIntegralConstVector(ssize_t constVal)
inline bool GenTree::IsIntegralConstVector(ssize_t constVal) const
{
#ifdef FEATURE_SIMD
// SIMDIntrinsicInit intrinsic with a const value as initializer
Expand All @@ -7008,7 +7008,7 @@ inline bool GenTree::IsIntegralConstVector(ssize_t constVal)
#ifdef FEATURE_HW_INTRINSICS
if (gtOper == GT_HWINTRINSIC)
{
GenTreeHWIntrinsic* node = AsHWIntrinsic();
const GenTreeHWIntrinsic* node = AsHWIntrinsic();

if (!varTypeIsIntegral(node->GetSimdBaseType()))
{
Expand Down Expand Up @@ -7058,7 +7058,7 @@ inline bool GenTree::IsIntegralConstVector(ssize_t constVal)
// Returns:
// True if this represents an integral const SIMD vector.
//
inline bool GenTree::IsSIMDZero()
inline bool GenTree::IsSIMDZero() const
{
#ifdef FEATURE_SIMD
if ((gtOper == GT_SIMD) && (AsSIMD()->gtSIMDIntrinsicID == SIMDIntrinsicInit))
Expand Down
53 changes: 52 additions & 1 deletion src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3120,6 +3120,7 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
}
if ((lclStore->TypeGet() == TYP_STRUCT) && !srcIsMultiReg && (src->OperGet() != GT_PHI))
{
bool convertToStoreObj;
if (src->OperGet() == GT_CALL)
{
GenTreeCall* call = src->AsCall();
Expand Down Expand Up @@ -3165,8 +3166,58 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
return;
}
#endif // !WINDOWS_AMD64_ABI
convertToStoreObj = false;
}
else if (!src->OperIs(GT_LCL_VAR) || (varDsc->GetLayout()->GetRegisterType() == TYP_UNDEF))
else if (!varDsc->IsEnregisterable())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main goal of this change is to keep ASG(LCL_VAR, 0) as STORE_LCL_VAR(0) without taking the address of the lcl.

{
convertToStoreObj = true;
}
else if (src->OperIs(GT_CNS_INT))
{
assert(src->IsIntegralConst(0) && "expected an INIT_VAL for non-zero init.");
var_types regType = varDsc->GetRegisterType();
#ifdef FEATURE_SIMD
if (varTypeIsSIMD(regType))
{
if (!varDsc->lvDoNotEnregister)
{
CorInfoType simdBaseJitType = comp->getBaseJitTypeOfSIMDLocal(lclStore);
if (simdBaseJitType == CORINFO_TYPE_UNDEF)
{
// Lie about the type if we don't know/have it.
simdBaseJitType = CORINFO_TYPE_FLOAT;
}
GenTreeSIMD* simdTree =
comp->gtNewSIMDNode(regType, src, SIMDIntrinsicInit, simdBaseJitType, varDsc->lvExactSize);
BlockRange().InsertAfter(src, simdTree);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we lower the newly created simdTree so containment and other transforms can happen as appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, thank you! It fixes a todo that I added because now we have SIMD16 instead of SIMD12.

src = simdTree;
lclStore->gtOp1 = src;
convertToStoreObj = false;
}
else
{
// the local is already known to be on stack, cheaper to initialize 0 there then use simd regs.
// TODO-CQ: keep STORE_LCL and convert to non-simd after LSRA.
convertToStoreObj = true;
}
}
else
#endif // FEATURE_SIMD
{
convertToStoreObj = false;
}
}
else if (!src->OperIs(GT_LCL_VAR))
{
convertToStoreObj = true;
}
else
{
assert(src->OperIs(GT_LCL_VAR));
convertToStoreObj = false;
}

if (convertToStoreObj)
{
GenTreeLclVar* addr = comp->gtNewLclVarAddrNode(lclStore->GetLclNum(), TYP_BYREF);

Expand Down
24 changes: 13 additions & 11 deletions src/coreclr/jit/rationalize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,23 +398,25 @@ void Rationalizer::RewriteAssignment(LIR::Use& use)
#ifdef FEATURE_SIMD
if (varTypeIsSIMD(location) && assignment->OperIsInitBlkOp())
{
if (location->OperGet() == GT_LCL_VAR)
if (location->OperIs(GT_LCL_VAR))
{
var_types simdType = location->TypeGet();
GenTree* initVal = assignment->AsOp()->gtOp2;
CorInfoType simdBaseJitType = comp->getBaseJitTypeOfSIMDLocal(location);
if (simdBaseJitType != CORINFO_TYPE_UNDEF)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have never had simdBaseJitType == CORINFO_TYPE_UNDEF on x64, otherwise it would be an assert later on

codegenxarch.cpp:4439:
assert(varTypeUsesFloatReg(targetType) == varTypeUsesFloatReg(op1Type));

from a tree like:

STORE_LCL_VAR simd16 V01
\--* CNS_INT 0

Copy link
Member

@tannergooding tannergooding May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is possible, but we handle this in register allocation by lying about the type:
https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/lsra.cpp#L6995-L7000

    if (simdNode->GetSimdBaseJitType() == CORINFO_TYPE_UNDEF)
    {
        // There are a few scenarios where we can get a LCL_VAR which
        // doesn't know the underlying baseType. In that scenario, we
        // will just lie and say it is a float. Codegen doesn't actually
        // care what the type is but this avoids an assert that would
        // otherwise be fired from the more general checks that happen.
        simdNode->SetSimdBaseJitType(CORINFO_TYPE_FLOAT);
    }

Namely there are a variety of scenarios where the base type might not get copied around for things like SIMD locals and where we lose the class handle and type information.

Ideally, we would preserve this 100% of the time, since its needed by things like CSE, but we don't today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The potential issue was:
ASG SIMD(LCL_VAR , 0) with simdBaseJitType == CORINFO_TYPE_UNDEF comes to rationalize and does not create STORE_LCL_VAR(SIMD(0)) leaving STORE_LCL_VAR(0).
code in lsra does not matter, it won't create a new node because it is after rationalize.
codegenxarch fails.

These lyings could be also deleted now (will push a change soon), maybe it will require to add if ((simdNode->gtSIMDIntrinsicID == SIMDIntrinsicUpperSave) || (simdNode->gtSIMDIntrinsicID != SIMDIntrinsicUpperRestore)) to simdcodegenxarch.cpp:genSIMDIntrinsic as we have this condition for arm64.

Copy link
Contributor

Choose a reason for hiding this comment

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

ASG SIMD(LCL_VAR , 0)

Is an interesting tree. In my testing (wrote a checker that looks for all ASG SIMD trees with mismatched types of LHS and RHS and ran it through SPMI), it only comes up when you initobj/initblk a SIMD local, and even then morph transforms it into ASG SIMD(LCL_VAR, SIMD(0)). I am curious to know if you know of other sources because my (WIP) fix for #51500 is to never import such ASGs in the first place.

Copy link
Contributor Author

@sandreenko sandreenko May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

oh, this code exists here to catch what morph did not for some reason, for example:

repro-7984.zip
unzip it and run like

"D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\superpmi.exe" D:\Sergey\git\runtime\artifacts\obj\coreclr\windows.x64.Checked\ide\jit\Debug\clrjit.dll(or any jit that you want to use) "repro-7984.mc" -jitoption force AltJit= -jitoption force AltJitNgen= -jitoption force JitDump=*   -jitoption force NgenDump=*

to see the dump, it will show you that in this case the issue is in fgMorphBlock that does not call morphTree for the trees that it creates, @kunalspathak recently pointed me to another issue caused by the same source.
I have decided not to hack fgMorphBlock anymore to squeeze positive diffs(and support struct enreg), so I am rewriting it now. However, it will take time and for this PR I want to keep this "catch morph failures" block in place.

fgMorphInitBlock: using field by field initialization.
GenTreeNode creates assertion:
               [000079] IA----------              *  ASG       simd12 (init)
In BB01 New Local Constant Assertion: V06 == 0 index=#01, mask=0000000000000001
GenTreeNode creates assertion:
               [000082] -A----------              *  ASG       float
In BB01 New Local Constant Assertion: V07 == 0.000000 index=#02, mask=0000000000000002
fgMorphInitBlock (after):
               [000083] -A---+------              *  COMMA     void
               [000079] IA----------              +--*  ASG       simd12 (init) <- forgot to call fgMorphTree :-( 
               [000077] D------N----              |  +--*  LCL_VAR   simd12<System.Numerics.Vector3> V06 tmp4
               [000078] ------------              |  \--*  CNS_INT   simd12 0
               [000082] -A----------              \--*  ASG       float
               [000080] D------N----                 +--*  LCL_VAR   float  V07 tmp5
               [000081] ------------                 \--*  CNS_DBL   float  0.00000000000000000

Copy link
Contributor

Choose a reason for hiding this comment

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

I have decided not to hack fgMorphBlock anymore to squeeze positive diffs(and support struct enreg), so I am rewriting it now. However, it will take time.

I believe you 😄. Good luck!

it will show you that in this case the issue is in fgMorphBlock that does not call morphTree for the trees that it creates

Thanks!

if (simdBaseJitType == CORINFO_TYPE_UNDEF)
{
GenTreeSIMD* simdTree = new (comp, GT_SIMD)
GenTreeSIMD(simdType, initVal, SIMDIntrinsicInit, simdBaseJitType, genTypeSize(simdType));
assignment->AsOp()->gtOp2 = simdTree;
value = simdTree;
initVal->gtNext = simdTree;
simdTree->gtPrev = initVal;

simdTree->gtNext = location;
location->gtPrev = simdTree;
// Lie about the type if we don't know/have it.
simdBaseJitType = CORINFO_TYPE_FLOAT;
}
GenTreeSIMD* simdTree =
comp->gtNewSIMDNode(simdType, initVal, SIMDIntrinsicInit, simdBaseJitType, genTypeSize(simdType));
assignment->AsOp()->gtOp2 = simdTree;
value = simdTree;
initVal->gtNext = simdTree;
simdTree->gtPrev = initVal;

simdTree->gtNext = location;
location->gtPrev = simdTree;
}
}
#endif // FEATURE_SIMD
Expand Down
6 changes: 1 addition & 5 deletions src/coreclr/jit/simdcodegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2353,11 +2353,7 @@ void CodeGen::genSIMDIntrinsicUpperRestore(GenTreeSIMD* simdNode)
void CodeGen::genSIMDIntrinsic(GenTreeSIMD* simdNode)
{
// NYI for unsupported base types
if (simdNode->GetSimdBaseType() != TYP_INT && simdNode->GetSimdBaseType() != TYP_LONG &&
simdNode->GetSimdBaseType() != TYP_FLOAT && simdNode->GetSimdBaseType() != TYP_DOUBLE &&
simdNode->GetSimdBaseType() != TYP_USHORT && simdNode->GetSimdBaseType() != TYP_UBYTE &&
simdNode->GetSimdBaseType() != TYP_SHORT && simdNode->GetSimdBaseType() != TYP_BYTE &&
simdNode->GetSimdBaseType() != TYP_UINT && simdNode->GetSimdBaseType() != TYP_ULONG)
if (!varTypeIsArithmetic(simdNode->GetSimdBaseType()))
{
noway_assert(!"SIMD intrinsic with unsupported base type.");
}
Expand Down