Skip to content

Commit 928bbee

Browse files
briansullbriansull
committed
Improved the fix and now it is an code size improvement
1 parent b9aa946 commit 928bbee

File tree

4 files changed

+88
-52
lines changed

4 files changed

+88
-52
lines changed

‎src/coreclr/src/jit/codegenarm.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -999,7 +999,11 @@ void CodeGen::genCodeForStoreLclFld(GenTreeLclFld* tree)
999999
assert(varNum < compiler->lvaCount);
10001000
LclVarDsc* varDsc = &(compiler->lvaTable[varNum]);
10011001

1002+
// Ensure that lclVar nodes are typed correctly.
1003+
assert(!varDsc->lvNormalizeOnStore() || targetType == genActualType(varDsc->TypeGet()));
1004+
10021005
GenTree* data = tree->gtOp1;
1006+
10031007
assert(!data->isContained());
10041008
genConsumeReg(data);
10051009
regNumber dataReg = data->GetRegNum();

‎src/coreclr/src/jit/codegenarm64.cpp‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1891,6 +1891,9 @@ void CodeGen::genCodeForStoreLclFld(GenTreeLclFld* tree)
18911891
assert(varNum < compiler->lvaCount);
18921892
LclVarDsc* varDsc = &(compiler->lvaTable[varNum]);
18931893

1894+
// Ensure that lclVar nodes are typed correctly.
1895+
assert(!varDsc->lvNormalizeOnStore() || targetType == genActualType(varDsc->TypeGet()));
1896+
18941897
GenTree* data = tree->gtOp1;
18951898
genConsumeRegs(data);
18961899

‎src/coreclr/src/jit/compiler.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -471,11 +471,11 @@ class LclVarDsc
471471
// 32-bit target. For implicit byref parameters, this gets hijacked between
472472
// fgRetypeImplicitByRefArgs and fgMarkDemotedImplicitByRefArgs to indicate whether
473473
// references to the arg are being rewritten as references to a promoted shadow local.
474-
unsigned char lvIsStructField : 1; // Is this local var a field of a promoted struct local?
475-
unsigned char lvOverlappingFields : 1; // True when we have a struct with possibly overlapping fields
476-
unsigned char lvContainsHoles : 1; // True when we have a promoted struct that contains holes
477-
unsigned char lvCustomLayout : 1; // True when this struct has "CustomLayout"
478-
unsigned char lvForceLoadNormalize : 1; // True when this local had a cast on the LHS of an assignment
474+
unsigned char lvIsStructField : 1; // Is this local var a field of a promoted struct local?
475+
unsigned char lvOverlappingFields : 1; // True when we have a struct with possibly overlapping fields
476+
unsigned char lvContainsHoles : 1; // True when we have a promoted struct that contains holes
477+
unsigned char lvCustomLayout : 1; // True when this struct has "CustomLayout"
478+
unsigned char lvForceLoadNormalize : 1; // True when this local had a cast on the LHS of an assignment
479479

480480
unsigned char lvIsMultiRegArg : 1; // true if this is a multireg LclVar struct used in an argument context
481481
unsigned char lvIsMultiRegRet : 1; // true if this is a multireg LclVar struct assigned from a multireg call

‎src/coreclr/src/jit/morph.cpp‎

Lines changed: 76 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -11642,14 +11642,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1164211642
// TODO-1stClassStructs: improve this.
1164311643
if (op1->IsLocal() || (op1->TypeGet() != TYP_STRUCT))
1164411644
{
11645+
// Consider using GTF_IND_ASG_LHS here, as GTF_DONT_CSE here is used to indicate
11646+
// that this GT_IND is the LHS of an assignment
11647+
//
1164511648
op1->gtFlags |= GTF_DONT_CSE;
11646-
11647-
// If the left side of the assignment is a GT_IND we mark with GTF_IND_ASG_LHS
11648-
if (op1->OperIs(GT_IND))
11649-
{
11650-
// Mark this GT_IND as a LHS, so that we know this is a store
11651-
op1->gtFlags |= GTF_IND_ASG_LHS;
11652-
}
1165311649
}
1165411650
break;
1165511651

@@ -13655,21 +13651,24 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1365513651
break;
1365613652
}
1365713653

13658-
bool foldAndReturnTemp;
13659-
bool isStore;
13654+
bool foldAndReturnTemp;
13655+
bool isStore;
13656+
var_types addCastToType;
1366013657

1366113658
foldAndReturnTemp = false;
13662-
isStore = (tree->gtFlags & GTF_IND_ASG_LHS) == GTF_IND_ASG_LHS;
13663-
temp = nullptr;
13664-
ival1 = 0;
13659+
isStore = (tree->gtFlags & GTF_DONT_CSE) == GTF_DONT_CSE;
13660+
addCastToType = TYP_VOID; // TYP_VOID means we don't need to insert a cast
13661+
13662+
temp = nullptr;
13663+
ival1 = 0;
1366513664

1366613665
// Don't remove a volatile GT_IND, even if the address points to a local variable.
1366713666
if ((tree->gtFlags & GTF_IND_VOLATILE) == 0)
1366813667
{
1366913668
/* Try to Fold *(&X) into X */
1367013669
if (op1->gtOper == GT_ADDR)
1367113670
{
13672-
// Can not remove a GT_ADDR if it is currently a CSE candidate.
13671+
// Cannot remove a GT_ADDR if it is currently a CSE candidate.
1367313672
if (gtIsActiveCSE_Candidate(op1))
1367413673
{
1367513674
break;
@@ -13716,7 +13715,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1371613715
}
1371713716
}
1371813717
}
13719-
bool matchingTypes = (lclType == typ);
13718+
bool matchingTypes = (lclType == typ);
13719+
bool matchingWidths = (genTypeSize(lclType) == genTypeSize(typ));
13720+
bool matchingFloat = (varTypeIsFloating(lclType) == varTypeIsFloating(typ));
1372013721

1372113722
// TYP_BOOL and TYP_UBYTE are also matching types
1372213723
if (!matchingTypes)
@@ -13731,34 +13732,43 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1373113732
}
1373213733
}
1373313734

13734-
// If the type of the Local Var matches the indirection type and we dont have a TYP_STRUCT,
13735-
// then often we can fold the IND/ADDR and use the LCL_VAR directly
13736-
if (matchingTypes && (typ != TYP_STRUCT))
13735+
// If the sizes match and we dont have a float or TYP_STRUCT,
13736+
// then we will fold the IND/ADDR and use the LCL_VAR directly
13737+
//
13738+
if (matchingWidths && matchingFloat && (typ != TYP_STRUCT))
1373713739
{
13738-
// For Loads or Stores of non small types (types that don't need sign or zero extends)
13739-
// we can fold the IND/ADDR and reduce to just the local variable
13740-
if (!varTypeIsSmall(typ))
13741-
{
13742-
tree->gtType = typ = temp->TypeGet();
13743-
foldAndReturnTemp = true;
13744-
}
13745-
else // varTypeIsSmall(typ) is true
13740+
// For some small types we might need to change to normalize loads or insert a cast here
13741+
//
13742+
if (varTypeIsSmall(typ))
1374613743
{
13747-
// For Loads of small types than are not NormalizeOnLoad()
13748-
// we can fold the IND/ADDR and reduce to just the local variable
13749-
//
13750-
// Stores of small types or when we don't normalize on load
13751-
// then we need to insert a cast
13744+
// For any stores of small types, we will force loads to be normalized
13745+
// this is necessary as we need to zero/sign extend any load
13746+
// after this kind of store.
1375213747
//
13753-
if (isStore || !varDsc->lvNormalizeOnLoad())
13748+
if (isStore)
1375413749
{
1375513750
varDsc->lvForceLoadNormalize = true;
1375613751
}
13757-
tree->gtType = typ = temp->TypeGet();
13758-
foldAndReturnTemp = true;
13752+
// otherwise we have a load operation
13753+
//
13754+
// Do we have a non matching small type load?
13755+
// we may need to insert a cast operation
13756+
else if (!matchingTypes)
13757+
{
13758+
if (!varDsc->lvNormalizeOnLoad())
13759+
{
13760+
// Since we aren't normalizing on a loads
13761+
// we need to insert a cast here.
13762+
//
13763+
addCastToType = typ;
13764+
}
13765+
}
1375913766
}
13767+
// we will fold the IND/ADDR and reduce to just the local variable
13768+
//
13769+
tree->gtType = typ = temp->TypeGet();
13770+
foldAndReturnTemp = true;
1376013771
}
13761-
1376213772
if (!foldAndReturnTemp)
1376313773
{
1376413774
// Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e.
@@ -13949,19 +13959,38 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1394913959
// normalization.
1395013960
if (temp->OperIs(GT_LCL_VAR))
1395113961
{
13952-
#ifdef DEBUG
13953-
// We clear this flag on `temp` because `fgMorphLocalVar` may assert that this bit is clear
13954-
// and the node in question must have this bit set (as it has already been morphed).
13955-
temp->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED;
13956-
#endif // DEBUG
13957-
const bool forceRemorph = true;
13958-
temp = fgMorphLocalVar(temp, forceRemorph);
13959-
#ifdef DEBUG
13960-
// We then set this flag on `temp` because `fgMorhpLocalVar` may not set it itself, and the
13961-
// caller of `fgMorphSmpOp` may assert that this flag is set on `temp` once this function
13962-
// returns.
13963-
temp->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
13964-
#endif // DEBUG
13962+
unsigned lclNum = temp->AsLclVarCommon()->GetLclNum();
13963+
var_types lclType = lvaTable[lclNum].lvType;
13964+
13965+
// We build a new 'result' tree to return, as we want to call fgMorphTree on it
13966+
//
13967+
GenTree* result = gtNewLclvNode(lclNum, lclType);
13968+
assert(result->OperGet() == GT_LCL_VAR);
13969+
13970+
// Copy the GTF_DONT_CSE flag from the original tree to `result`
13971+
// because fgMorphLocalVar uses this flag to determine if we are loading or storing
13972+
// and will insert a cast when we are loading from a lvNormalizeOnLoad() local
13973+
//
13974+
result->gtFlags |= (tree->gtFlags & GTF_DONT_CSE);
13975+
13976+
// If the indirection node was a different small type than our local variable
13977+
// we insert a cast to that type.
13978+
//
13979+
if (addCastToType != TYP_VOID)
13980+
{
13981+
assert(varTypeIsSmall(addCastToType));
13982+
13983+
// Insert a narrowing cast to the old indirection type
13984+
//
13985+
result = gtNewCastNode(TYP_INT, result, false, addCastToType);
13986+
}
13987+
13988+
// On this path we return 'result' after calling fgMorphTree on it.
13989+
// so now we can destroy the unused'temp' node.
13990+
//
13991+
DEBUG_DESTROY_NODE(temp);
13992+
13993+
temp = fgMorphTree(result);
1396513994
}
1396613995

1396713996
return temp;

0 commit comments

Comments
 (0)