Skip to content

Clean up fix for single-reg returned normalize-on-load vars #59645

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

Merged
jakobbotsch merged 2 commits into from
Oct 2, 2021
Merged

Clean up fix for single-reg returned normalize-on-load vars #59645

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
e9a3a6f
Clean up fix for single-reg returned normalize-on-load vars
jakobbotsch Sep 27, 2021
5d83990
Allow cast assertions for normalize-on-load vars in global assertion …
jakobbotsch Sep 27, 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
Next Next commit
Clean up fix for single-reg returned normalize-on-load vars 
This is a better fix for #58373 that changes the handling of this to
happen in morph for all cases. We sometimes missed the insertion of
necessary casts because we forgot to remove a GTF_DONT_CSE flag when
folding an indirection. Fixing this leads to some new GT_CNS_DBL cases
in lowering that hit an assert, but those cases should be correctly
handled by the default case so just remove the assert.

To get rid of some of the regressions I have allowed generating
assertions when assigning struct fields from casts. It was unclear why
this was not allowed in the first place.
  • Loading branch information
@jakobbotsch
jakobbotsch committed Sep 27, 2021
commit e9a3a6f93df3ae55baba41e04bf3ae0414eadca2
11 changes: 2 additions & 9 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,21 +1547,14 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1,
// Try and see if we can make a subrange assertion.
if (((assertionKind == OAK_SUBRANGE) || (assertionKind == OAK_EQUAL)))
{
// Keep the casts on small struct fields.
Copy link
Contributor

@SingleAccretion SingleAccretion Sep 27, 2021

Choose a reason for hiding this comment

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

You can also delete this from the global propagation code too, I presume. Though we do not have practically any test coverage for that code.

Copy link
Member Author

@jakobbotsch jakobbotsch Sep 27, 2021

Choose a reason for hiding this comment

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

Done, it did not result in any diffs except for when it pushed out some other assertions.

// TODO-Review: why?
if (op2->OperIs(GT_CAST) && lvaTable[lclNum].lvIsStructField && lvaTable[lclNum].lvNormalizeOnLoad())
{
goto DONE_ASSERTION;
}

if (optTryExtractSubrangeAssertion(op2, &assertion.op2.u2))
{
assertion.op2.kind = O2K_SUBRANGE;
assertion.assertionKind = OAK_SUBRANGE;
}
}
} // else // !helperCallArgs
} // if (op1->gtOper == GT_LCL_VAR)
}
}

//
// Are we making an IsType assertion?
Expand Down
39 changes: 1 addition & 38 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3371,16 +3371,6 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret)
}
break;

#if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_ARM)
case GT_CNS_DBL:
// Currently we are not promoting structs with a single float field,
// https://github.com/dotnet/runtime/issues/4323

// TODO-CQ: can improve `GT_CNS_DBL` handling for supported platforms, but
// because it is only x86 nowadays it is not worth it.
unreached();
#endif

default:
assert(varTypeIsEnregisterable(retVal));
if (varTypeUsesFloatReg(ret) != varTypeUsesFloatReg(retVal))
Expand Down Expand Up @@ -3414,24 +3404,7 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret)
unsigned lclNum = lclVar->GetLclNum();
LclVarDsc* varDsc = comp->lvaGetDesc(lclNum);

bool replacedInLowering = false;
if (varDsc->CanBeReplacedWithItsField(comp))
{
// We can replace the struct with its only field and keep the field on a register.
unsigned fieldLclNum = varDsc->lvFieldLclStart;
LclVarDsc* fieldDsc = comp->lvaGetDesc(fieldLclNum);
assert(varTypeIsSmallInt(fieldDsc->lvType)); // For a non-small type it had to be done in morph.

lclVar->SetLclNum(fieldLclNum);
JITDUMP("Replacing an independently promoted local var V%02u with its only field V%02u for the return "
"[%06u]\n",
lclNum, fieldLclNum, comp->dspTreeID(ret));
lclVar->ChangeType(fieldDsc->lvType);
lclNum = fieldLclNum;
varDsc = comp->lvaGetDesc(lclNum);
replacedInLowering = true;
}
else if (varDsc->lvPromoted)
if (varDsc->lvPromoted)
{
// TODO-1stClassStructs: We can no longer independently promote
// or enregister this struct, since it is referenced as a whole.
Expand Down Expand Up @@ -3469,16 +3442,6 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret)
const var_types lclVarType = varDsc->GetRegisterType(lclVar);
assert(lclVarType != TYP_UNDEF);

if (varDsc->lvNormalizeOnLoad() && replacedInLowering)
{
// For a normalize-on-load var that we replaced late we need to insert a cast
// as morph would typically be responsible for this.
GenTreeCast* cast = comp->gtNewCastNode(TYP_INT, lclVar, false, lclVarType);
ret->gtOp1 = cast;
BlockRange().InsertBefore(ret, cast);
ContainCheckCast(cast);
}

const var_types actualType = genActualType(lclVarType);
lclVar->ChangeType(actualType);

Expand Down
31 changes: 17 additions & 14 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6000,7 +6000,11 @@ GenTree* Compiler::fgMorphLocalVar(GenTree* tree, bool forceRemorph)
// TODO-CQ: fix the VNs for normalize-on-load locals and remove this quirk.
bool isBoolQuirk = varType == TYP_BOOL;

// Assertion prop can tell us to omit adding a cast here.
// Assertion prop can tell us to omit adding a cast here. This is
// useful when the local is a small-typed parameter that is passed in a
// register: in that case, the ABI specifies that the upper bits might
// be invalid, but the assertion guarantees us that we have normalized
// when we wrote it.
if (optLocalAssertionProp && !isBoolQuirk &&
optAssertionIsSubrange(tree, IntegralRange::ForType(varType), apFull) != NO_ASSERTION_INDEX)
{
Expand Down Expand Up @@ -11565,18 +11569,12 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
unsigned fieldLclNum = varDsc->lvFieldLclStart;
LclVarDsc* fieldDsc = lvaGetDesc(fieldLclNum);

if (!varTypeIsSmallInt(fieldDsc->lvType))
{
// TODO-CQ: support that substitution for small types without creating `CAST` node.
// When a small struct is returned in a register higher bits could be left in undefined
// state.
JITDUMP("Replacing an independently promoted local var V%02u with its only field "
"V%02u for "
"the return [%06u]\n",
lclVar->GetLclNum(), fieldLclNum, dspTreeID(tree));
lclVar->SetLclNum(fieldLclNum);
lclVar->ChangeType(fieldDsc->lvType);
}
JITDUMP("Replacing an independently promoted local var V%02u with its only field "
"V%02u for "
"the return [%06u]\n",
lclVar->GetLclNum(), fieldLclNum, dspTreeID(tree));
lclVar->SetLclNum(fieldLclNum);
lclVar->ChangeType(fieldDsc->lvType);
}
}
}
Expand Down Expand Up @@ -13929,7 +13927,12 @@ GenTree* Compiler::fgMorphRetInd(GenTreeUnOp* ret)
DEBUG_DESTROY_NODE(ind);
DEBUG_DESTROY_NODE(addr);
ret->gtOp1 = lclVar;
return ret->gtGetOp1();
// We use GTF_DONT_CSE as an "is under GT_ADDR" check. We can
// get rid of it now since the GT_RETURN node should never have
// its address taken.
assert((ret->gtFlags & GTF_DONT_CSE) == 0);
lclVar->gtFlags &= ~GTF_DONT_CSE;
return lclVar;
}
else if (!varDsc->lvDoNotEnregister)
{
Expand Down