From 3e216841102ab82a6f4b276aadd8558d78819c12 Mon Sep 17 00:00:00 2001 From: mikedn Date: Fri, 21 Dec 2018 02:10:27 +0200 Subject: [PATCH] Don't morph volatile IND(ADDR(LCL_VAR)) (#20843) Besides the fact that volatile indirections aren't supposed to be removed doing so in this case results in incorrect code being generated. The GT_IND node has GTF_DONT_CSE set and this gets copied to the GT_LCL_VAR node. Later this prevents the insertion of a normalization cast because GTF_DONT_CSE on a GT_LCL_VAR node is assumed to mean that the variable address is being taken. --- src/jit/morph.cpp | 260 +++++++++++---------- .../JitBlue/GitHub_19599/GitHub_19599.cs | 30 +++ .../JitBlue/GitHub_19599/GitHub_19599.csproj | 17 ++ 3 files changed, 182 insertions(+), 125 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.csproj diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index ebe4105..0ee1927 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -13088,166 +13088,170 @@ DONE_MORPHING_CHILDREN: temp = nullptr; ival1 = 0; - /* Try to Fold *(&X) into X */ - if (op1->gtOper == GT_ADDR) + // Don't remove a volatile GT_IND, even if the address points to a local variable. + if ((tree->gtFlags & GTF_IND_VOLATILE) == 0) { - // Can not remove a GT_ADDR if it is currently a CSE candidate. - if (gtIsActiveCSE_Candidate(op1)) + /* Try to Fold *(&X) into X */ + if (op1->gtOper == GT_ADDR) { - break; - } - - temp = op1->gtOp.gtOp1; // X + // Can not remove a GT_ADDR if it is currently a CSE candidate. + if (gtIsActiveCSE_Candidate(op1)) + { + break; + } - // In the test below, if they're both TYP_STRUCT, this of course does *not* mean that - // they are the *same* struct type. In fact, they almost certainly aren't. If the - // address has an associated field sequence, that identifies this case; go through - // the "lcl_fld" path rather than this one. - FieldSeqNode* addrFieldSeq = nullptr; // This is an unused out parameter below. - if (typ == temp->TypeGet() && !GetZeroOffsetFieldMap()->Lookup(op1, &addrFieldSeq)) - { - foldAndReturnTemp = true; - } - else if (temp->OperIsLocal()) - { - unsigned lclNum = temp->gtLclVarCommon.gtLclNum; - LclVarDsc* varDsc = &lvaTable[lclNum]; + temp = op1->gtOp.gtOp1; // X - // We will try to optimize when we have a promoted struct promoted with a zero lvFldOffset - if (varDsc->lvPromoted && (varDsc->lvFldOffset == 0)) + // In the test below, if they're both TYP_STRUCT, this of course does *not* mean that + // they are the *same* struct type. In fact, they almost certainly aren't. If the + // address has an associated field sequence, that identifies this case; go through + // the "lcl_fld" path rather than this one. + FieldSeqNode* addrFieldSeq = nullptr; // This is an unused out parameter below. + if (typ == temp->TypeGet() && !GetZeroOffsetFieldMap()->Lookup(op1, &addrFieldSeq)) { - noway_assert(varTypeIsStruct(varDsc)); + foldAndReturnTemp = true; + } + else if (temp->OperIsLocal()) + { + unsigned lclNum = temp->gtLclVarCommon.gtLclNum; + LclVarDsc* varDsc = &lvaTable[lclNum]; - // We will try to optimize when we have a single field struct that is being struct promoted - if (varDsc->lvFieldCnt == 1) + // We will try to optimize when we have a promoted struct promoted with a zero lvFldOffset + if (varDsc->lvPromoted && (varDsc->lvFldOffset == 0)) { - unsigned lclNumFld = varDsc->lvFieldLclStart; - // just grab the promoted field - LclVarDsc* fieldVarDsc = &lvaTable[lclNumFld]; + noway_assert(varTypeIsStruct(varDsc)); - // Also make sure that the tree type matches the fieldVarType and that it's lvFldOffset - // is zero - if (fieldVarDsc->TypeGet() == typ && (fieldVarDsc->lvFldOffset == 0)) + // We will try to optimize when we have a single field struct that is being struct promoted + if (varDsc->lvFieldCnt == 1) { - // We can just use the existing promoted field LclNum - temp->gtLclVarCommon.SetLclNum(lclNumFld); - temp->gtType = fieldVarDsc->TypeGet(); + unsigned lclNumFld = varDsc->lvFieldLclStart; + // just grab the promoted field + LclVarDsc* fieldVarDsc = &lvaTable[lclNumFld]; - foldAndReturnTemp = true; + // Also make sure that the tree type matches the fieldVarType and that it's lvFldOffset + // is zero + if (fieldVarDsc->TypeGet() == typ && (fieldVarDsc->lvFldOffset == 0)) + { + // We can just use the existing promoted field LclNum + temp->gtLclVarCommon.SetLclNum(lclNumFld); + temp->gtType = fieldVarDsc->TypeGet(); + + foldAndReturnTemp = true; + } } } - } - // If the type of the IND (typ) is a "small int", and the type of the local has the - // same width, then we can reduce to just the local variable -- it will be - // correctly normalized, and signed/unsigned differences won't matter. - // - // The below transformation cannot be applied if the local var needs to be normalized on load. - else if (varTypeIsSmall(typ) && (genTypeSize(lvaTable[lclNum].lvType) == genTypeSize(typ)) && - !lvaTable[lclNum].lvNormalizeOnLoad()) - { - tree->gtType = typ = temp->TypeGet(); - foldAndReturnTemp = true; - } - else if (!varTypeIsStruct(typ) && (lvaTable[lclNum].lvType == typ) && - !lvaTable[lclNum].lvNormalizeOnLoad()) - { - tree->gtType = typ = temp->TypeGet(); - foldAndReturnTemp = true; - } - else - { - // Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e. - // nullptr) - assert(fieldSeq == nullptr); - bool b = GetZeroOffsetFieldMap()->Lookup(op1, &fieldSeq); - assert(b || fieldSeq == nullptr); - - if ((fieldSeq != nullptr) && (temp->OperGet() == GT_LCL_FLD)) + // If the type of the IND (typ) is a "small int", and the type of the local has the + // same width, then we can reduce to just the local variable -- it will be + // correctly normalized, and signed/unsigned differences won't matter. + // + // The below transformation cannot be applied if the local var needs to be normalized on load. + else if (varTypeIsSmall(typ) && (genTypeSize(lvaTable[lclNum].lvType) == genTypeSize(typ)) && + !lvaTable[lclNum].lvNormalizeOnLoad()) { - // Append the field sequence, change the type. - temp->AsLclFld()->gtFieldSeq = - GetFieldSeqStore()->Append(temp->AsLclFld()->gtFieldSeq, fieldSeq); - temp->gtType = typ; - - foldAndReturnTemp = true; + tree->gtType = typ = temp->TypeGet(); + foldAndReturnTemp = true; } - } - // Otherwise will will fold this into a GT_LCL_FLD below - // where we check (temp != nullptr) - } - else // !temp->OperIsLocal() - { - // We don't try to fold away the GT_IND/GT_ADDR for this case - temp = nullptr; - } - } - else if (op1->OperGet() == GT_ADD) - { - /* Try to change *(&lcl + cns) into lcl[cns] to prevent materialization of &lcl */ + else if (!varTypeIsStruct(typ) && (lvaTable[lclNum].lvType == typ) && + !lvaTable[lclNum].lvNormalizeOnLoad()) + { + tree->gtType = typ = temp->TypeGet(); + foldAndReturnTemp = true; + } + else + { + // Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e. + // nullptr) + assert(fieldSeq == nullptr); + bool b = GetZeroOffsetFieldMap()->Lookup(op1, &fieldSeq); + assert(b || fieldSeq == nullptr); - if (op1->gtOp.gtOp1->OperGet() == GT_ADDR && op1->gtOp.gtOp2->OperGet() == GT_CNS_INT && - (!(opts.MinOpts() || opts.compDbgCode))) - { - // No overflow arithmetic with pointers - noway_assert(!op1->gtOverflow()); + if ((fieldSeq != nullptr) && (temp->OperGet() == GT_LCL_FLD)) + { + // Append the field sequence, change the type. + temp->AsLclFld()->gtFieldSeq = + GetFieldSeqStore()->Append(temp->AsLclFld()->gtFieldSeq, fieldSeq); + temp->gtType = typ; - temp = op1->gtOp.gtOp1->gtOp.gtOp1; - if (!temp->OperIsLocal()) - { - temp = nullptr; - break; + foldAndReturnTemp = true; + } + } + // Otherwise will will fold this into a GT_LCL_FLD below + // where we check (temp != nullptr) } - - // Can not remove the GT_ADDR if it is currently a CSE candidate. - if (gtIsActiveCSE_Candidate(op1->gtOp.gtOp1)) + else // !temp->OperIsLocal() { - break; + // We don't try to fold away the GT_IND/GT_ADDR for this case + temp = nullptr; } + } + else if (op1->OperGet() == GT_ADD) + { + /* Try to change *(&lcl + cns) into lcl[cns] to prevent materialization of &lcl */ - ival1 = op1->gtOp.gtOp2->gtIntCon.gtIconVal; - fieldSeq = op1->gtOp.gtOp2->gtIntCon.gtFieldSeq; - - // Does the address have an associated zero-offset field sequence? - FieldSeqNode* addrFieldSeq = nullptr; - if (GetZeroOffsetFieldMap()->Lookup(op1->gtOp.gtOp1, &addrFieldSeq)) + if (op1->gtOp.gtOp1->OperGet() == GT_ADDR && op1->gtOp.gtOp2->OperGet() == GT_CNS_INT && + (!(opts.MinOpts() || opts.compDbgCode))) { - fieldSeq = GetFieldSeqStore()->Append(addrFieldSeq, fieldSeq); - } + // No overflow arithmetic with pointers + noway_assert(!op1->gtOverflow()); - if (ival1 == 0 && typ == temp->TypeGet() && temp->TypeGet() != TYP_STRUCT) - { - noway_assert(!varTypeIsGC(temp->TypeGet())); - foldAndReturnTemp = true; - } - else - { - // The emitter can't handle large offsets - if (ival1 != (unsigned short)ival1) + temp = op1->gtOp.gtOp1->gtOp.gtOp1; + if (!temp->OperIsLocal()) { + temp = nullptr; break; } - // The emitter can get confused by invalid offsets - if (ival1 >= Compiler::lvaLclSize(temp->gtLclVarCommon.gtLclNum)) + // Can not remove the GT_ADDR if it is currently a CSE candidate. + if (gtIsActiveCSE_Candidate(op1->gtOp.gtOp1)) { break; } -#ifdef _TARGET_ARM_ - // Check for a LclVar TYP_STRUCT with misalignment on a Floating Point field - // - if (varTypeIsFloating(typ)) + ival1 = op1->gtOp.gtOp2->gtIntCon.gtIconVal; + fieldSeq = op1->gtOp.gtOp2->gtIntCon.gtFieldSeq; + + // Does the address have an associated zero-offset field sequence? + FieldSeqNode* addrFieldSeq = nullptr; + if (GetZeroOffsetFieldMap()->Lookup(op1->gtOp.gtOp1, &addrFieldSeq)) + { + fieldSeq = GetFieldSeqStore()->Append(addrFieldSeq, fieldSeq); + } + + if (ival1 == 0 && typ == temp->TypeGet() && temp->TypeGet() != TYP_STRUCT) + { + noway_assert(!varTypeIsGC(temp->TypeGet())); + foldAndReturnTemp = true; + } + else { - if ((ival1 % emitTypeSize(typ)) != 0) + // The emitter can't handle large offsets + if (ival1 != (unsigned short)ival1) { - tree->gtFlags |= GTF_IND_UNALIGNED; break; } - } + + // The emitter can get confused by invalid offsets + if (ival1 >= Compiler::lvaLclSize(temp->gtLclVarCommon.gtLclNum)) + { + break; + } + +#ifdef _TARGET_ARM_ + // Check for a LclVar TYP_STRUCT with misalignment on a Floating Point field + // + if (varTypeIsFloating(typ)) + { + if ((ival1 % emitTypeSize(typ)) != 0) + { + tree->gtFlags |= GTF_IND_UNALIGNED; + break; + } + } #endif + } + // Now we can fold this into a GT_LCL_FLD below + // where we check (temp != nullptr) } - // Now we can fold this into a GT_LCL_FLD below - // where we check (temp != nullptr) } } @@ -18205,7 +18209,13 @@ public: assert(TopValue(1).Node() == node); assert(TopValue(0).Node() == node->gtGetOp1()); - if (!TopValue(1).Indir(TopValue(0))) + if ((node->gtFlags & GTF_IND_VOLATILE) != 0) + { + // Volatile indirections must not be removed so the address, + // if any, must be escaped. + EscapeValue(TopValue(0), node); + } + else if (!TopValue(1).Indir(TopValue(0))) { // If the address comes from another indirection (e.g. IND(IND(...)) // then we need to escape the location. diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.cs b/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.cs new file mode 100644 index 0000000..2705d45 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.cs @@ -0,0 +1,30 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Runtime.CompilerServices; +using System.Threading; + +struct S0 +{ + public byte F0; +} + +class Program +{ + static S0 s_2; + static long s_5; + + static int Main() + { + s_2.F0 = 128; + M7(s_2); + return (s_5 == 128) ? 100 : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void M7(S0 arg0) + { + s_5 = Volatile.Read(ref arg0.F0); + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.csproj new file mode 100644 index 0000000..c24f74b --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.csproj @@ -0,0 +1,17 @@ + + + + + Release + AnyCPU + $(MSBuildProjectName) + Exe + + True + + + + + + + -- 2.7.4