From 928bbee4822daf7d75f9a75ea8d36913e63ff05e Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Mon, 3 Aug 2020 19:55:38 -0700 Subject: [PATCH] Improved the fix and now it is an code size improvement --- src/coreclr/src/jit/codegenarm.cpp | 4 + src/coreclr/src/jit/codegenarm64.cpp | 3 + src/coreclr/src/jit/compiler.h | 10 +-- src/coreclr/src/jit/morph.cpp | 123 +++++++++++++++++---------- 4 files changed, 88 insertions(+), 52 deletions(-) diff --git a/src/coreclr/src/jit/codegenarm.cpp b/src/coreclr/src/jit/codegenarm.cpp index b019d99df0dec7..2eaa80862396dd 100644 --- a/src/coreclr/src/jit/codegenarm.cpp +++ b/src/coreclr/src/jit/codegenarm.cpp @@ -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(); diff --git a/src/coreclr/src/jit/codegenarm64.cpp b/src/coreclr/src/jit/codegenarm64.cpp index c8af9cdeea0f23..a85e2ce06bf957 100644 --- a/src/coreclr/src/jit/codegenarm64.cpp +++ b/src/coreclr/src/jit/codegenarm64.cpp @@ -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); diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 92e31121052f04..a9e6ecbd0c7465 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -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 diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 8448adc79162e9..73d6cd8ff8f007 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -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; @@ -13655,13 +13651,16 @@ 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) @@ -13669,7 +13668,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) /* 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; @@ -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) @@ -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. @@ -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;