Skip to content

Commit

Permalink
Improved the fix and now it is an code size improvement
Browse files Browse the repository at this point in the history
  • Loading branch information
@briansull
briansull committed Aug 4, 2020
1 parent b9aa946 commit 928bbee
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 52 deletions.
4 changes: 4 additions & 0 deletions src/coreclr/src/jit/codegenarm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,11 @@ void CodeGen::genCodeForStoreLclFld(GenTreeLclFld* tree)
assert(varNum < compiler->lvaCount);
LclVarDsc* varDsc = &(compiler->lvaTable[varNum]);

// Ensure that lclVar nodes are typed correctly.
assert(!varDsc->lvNormalizeOnStore() || targetType == genActualType(varDsc->TypeGet()));

GenTree* data = tree->gtOp1;

assert(!data->isContained());
genConsumeReg(data);
regNumber dataReg = data->GetRegNum();
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/src/jit/codegenarm64.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1891,6 +1891,9 @@ void CodeGen::genCodeForStoreLclFld(GenTreeLclFld* tree)
assert(varNum < compiler->lvaCount);
LclVarDsc* varDsc = &(compiler->lvaTable[varNum]);

// Ensure that lclVar nodes are typed correctly.
assert(!varDsc->lvNormalizeOnStore() || targetType == genActualType(varDsc->TypeGet()));

GenTree* data = tree->gtOp1;
genConsumeRegs(data);

Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/src/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,11 +471,11 @@ class LclVarDsc
// 32-bit target. For implicit byref parameters, this gets hijacked between
// fgRetypeImplicitByRefArgs and fgMarkDemotedImplicitByRefArgs to indicate whether
// references to the arg are being rewritten as references to a promoted shadow local.
unsigned char lvIsStructField : 1; // Is this local var a field of a promoted struct local?
unsigned char lvOverlappingFields : 1; // True when we have a struct with possibly overlapping fields
unsigned char lvContainsHoles : 1; // True when we have a promoted struct that contains holes
unsigned char lvCustomLayout : 1; // True when this struct has "CustomLayout"
unsigned char lvForceLoadNormalize : 1; // True when this local had a cast on the LHS of an assignment
unsigned char lvIsStructField : 1; // Is this local var a field of a promoted struct local?
unsigned char lvOverlappingFields : 1; // True when we have a struct with possibly overlapping fields
unsigned char lvContainsHoles : 1; // True when we have a promoted struct that contains holes
unsigned char lvCustomLayout : 1; // True when this struct has "CustomLayout"
unsigned char lvForceLoadNormalize : 1; // True when this local had a cast on the LHS of an assignment

unsigned char lvIsMultiRegArg : 1; // true if this is a multireg LclVar struct used in an argument context
unsigned char lvIsMultiRegRet : 1; // true if this is a multireg LclVar struct assigned from a multireg call
Expand Down
123 changes: 76 additions & 47 deletions src/coreclr/src/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11642,14 +11642,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
// TODO-1stClassStructs: improve this.
if (op1->IsLocal() || (op1->TypeGet() != TYP_STRUCT))
{
// Consider using GTF_IND_ASG_LHS here, as GTF_DONT_CSE here is used to indicate
// that this GT_IND is the LHS of an assignment
//
op1->gtFlags |= GTF_DONT_CSE;

// If the left side of the assignment is a GT_IND we mark with GTF_IND_ASG_LHS
if (op1->OperIs(GT_IND))
{
// Mark this GT_IND as a LHS, so that we know this is a store
op1->gtFlags |= GTF_IND_ASG_LHS;
}
}
break;

Expand Down Expand Up @@ -13655,21 +13651,24 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
break;
}

bool foldAndReturnTemp;
bool isStore;
bool foldAndReturnTemp;
bool isStore;
var_types addCastToType;

foldAndReturnTemp = false;
isStore = (tree->gtFlags & GTF_IND_ASG_LHS) == GTF_IND_ASG_LHS;
temp = nullptr;
ival1 = 0;
isStore = (tree->gtFlags & GTF_DONT_CSE) == GTF_DONT_CSE;
addCastToType = TYP_VOID; // TYP_VOID means we don't need to insert a cast

temp = nullptr;
ival1 = 0;

// Don't remove a volatile GT_IND, even if the address points to a local variable.
if ((tree->gtFlags & GTF_IND_VOLATILE) == 0)
{
/* Try to Fold *(&X) into X */
if (op1->gtOper == GT_ADDR)
{
// Can not remove a GT_ADDR if it is currently a CSE candidate.
// Cannot remove a GT_ADDR if it is currently a CSE candidate.
if (gtIsActiveCSE_Candidate(op1))
{
break;
Expand Down Expand Up @@ -13716,7 +13715,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
}
}
}
bool matchingTypes = (lclType == typ);
bool matchingTypes = (lclType == typ);
bool matchingWidths = (genTypeSize(lclType) == genTypeSize(typ));
bool matchingFloat = (varTypeIsFloating(lclType) == varTypeIsFloating(typ));

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

// If the type of the Local Var matches the indirection type and we dont have a TYP_STRUCT,
// then often we can fold the IND/ADDR and use the LCL_VAR directly
if (matchingTypes && (typ != TYP_STRUCT))
// If the sizes match and we dont have a float or TYP_STRUCT,
// then we will fold the IND/ADDR and use the LCL_VAR directly
//
if (matchingWidths && matchingFloat && (typ != TYP_STRUCT))
{
// For Loads or Stores of non small types (types that don't need sign or zero extends)
// we can fold the IND/ADDR and reduce to just the local variable
if (!varTypeIsSmall(typ))
{
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
}
else // varTypeIsSmall(typ) is true
// For some small types we might need to change to normalize loads or insert a cast here
//
if (varTypeIsSmall(typ))
{
// For Loads of small types than are not NormalizeOnLoad()
// we can fold the IND/ADDR and reduce to just the local variable
//
// Stores of small types or when we don't normalize on load
// then we need to insert a cast
// For any stores of small types, we will force loads to be normalized
// this is necessary as we need to zero/sign extend any load
// after this kind of store.
//
if (isStore || !varDsc->lvNormalizeOnLoad())
if (isStore)
{
varDsc->lvForceLoadNormalize = true;
}
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
// otherwise we have a load operation
//
// Do we have a non matching small type load?
// we may need to insert a cast operation
else if (!matchingTypes)
{
if (!varDsc->lvNormalizeOnLoad())
{
// Since we aren't normalizing on a loads
// we need to insert a cast here.
//
addCastToType = typ;
}
}
}
// we will fold the IND/ADDR and reduce to just the local variable
//
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
}

if (!foldAndReturnTemp)
{
// Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e.
Expand Down Expand Up @@ -13949,19 +13959,38 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
// normalization.
if (temp->OperIs(GT_LCL_VAR))
{
#ifdef DEBUG
// We clear this flag on `temp` because `fgMorphLocalVar` may assert that this bit is clear
// and the node in question must have this bit set (as it has already been morphed).
temp->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED;
#endif // DEBUG
const bool forceRemorph = true;
temp = fgMorphLocalVar(temp, forceRemorph);
#ifdef DEBUG
// We then set this flag on `temp` because `fgMorhpLocalVar` may not set it itself, and the
// caller of `fgMorphSmpOp` may assert that this flag is set on `temp` once this function
// returns.
temp->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
#endif // DEBUG
unsigned lclNum = temp->AsLclVarCommon()->GetLclNum();
var_types lclType = lvaTable[lclNum].lvType;

// We build a new 'result' tree to return, as we want to call fgMorphTree on it
//
GenTree* result = gtNewLclvNode(lclNum, lclType);
assert(result->OperGet() == GT_LCL_VAR);

// Copy the GTF_DONT_CSE flag from the original tree to `result`
// because fgMorphLocalVar uses this flag to determine if we are loading or storing
// and will insert a cast when we are loading from a lvNormalizeOnLoad() local
//
result->gtFlags |= (tree->gtFlags & GTF_DONT_CSE);

// If the indirection node was a different small type than our local variable
// we insert a cast to that type.
//
if (addCastToType != TYP_VOID)
{
assert(varTypeIsSmall(addCastToType));

// Insert a narrowing cast to the old indirection type
//
result = gtNewCastNode(TYP_INT, result, false, addCastToType);
}

// On this path we return 'result' after calling fgMorphTree on it.
// so now we can destroy the unused'temp' node.
//
DEBUG_DESTROY_NODE(temp);

temp = fgMorphTree(result);
}

return temp;
Expand Down

0 comments on commit 928bbee

Please sign in to comment.