From efd5fa811e5b56a6baaa3047fe91a70b010a1a0d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 2 Mar 2020 18:06:04 -0800 Subject: [PATCH 1/6] JIT: remove GTF_INX_REFARR_LAYOUT When morphing `GT_INDEX` nodes, we were inadvertently also setting `GTF_IND_NONFAULTING` for the `GT_IND` subtree for ref type arrays, because `GTF_IND_NONFAULTING` has the same value as `GTF_INX_REFARR_LAYOUT`. This turns out to be safe since in general there is an upstream bounds check to cover the null check from the indexing operation, so the fact that we were claiming the `GT_IND` can't fault is ok. A no diff change would remove the `GTF_INX_REFARR_LAYOUT` flag and then modify `fgMorphArrayIndex` to set `GTF_IND_NONFAULTING` for ref type arrays with bounds checks: ``` // If there's a bounds check, the the indir won't fault. if (bndsChk && (tree->gtType == TYP_REF)) { tree->gtFlags |= GTF_IND_NONFAULTING; } tree->gtFlags |= GTF_EXCEPT; ``` But there's no good reason to limit the above change to ref type arrays and no good reason to OR in `GTF_EXCEPT` when there are bounds checks. Once we do the more general fix we see diffs, so we might as well further clean up the related constraint in the importer found under `REDO_RETURN_NODE`. Closes #32647. --- src/coreclr/src/jit/compiler.cpp | 4 ---- src/coreclr/src/jit/gentree.cpp | 12 ------------ src/coreclr/src/jit/gentree.h | 6 ------ src/coreclr/src/jit/importer.cpp | 10 +--------- src/coreclr/src/jit/morph.cpp | 11 +++++++++-- 5 files changed, 10 insertions(+), 33 deletions(-) diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index 60050c6c493b8d..35c41ca961c9a4 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -8648,10 +8648,6 @@ void cTreeFlags(Compiler* comp, GenTree* tree) case GT_INDEX: - if (tree->gtFlags & GTF_INX_REFARR_LAYOUT) - { - chars += printf("[INX_REFARR_LAYOUT]"); - } if (tree->gtFlags & GTF_INX_STRING_LAYOUT) { chars += printf("[INX_STRING_LAYOUT]"); diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 9c78cc1bc1dea3..7680ddeaaf0726 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -9773,18 +9773,6 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, __in __in_z _ case GT_INDEX: case GT_INDEX_ADDR: - - if ((tree->gtFlags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED)) == 0) // We prefer printing V or U over R - { - if (tree->gtFlags & GTF_INX_REFARR_LAYOUT) - { - printf("R"); - --msgLength; - break; - } // R means RefArray - } - __fallthrough; - case GT_FIELD: case GT_CLS_VAR: if (tree->gtFlags & GTF_IND_VOLATILE) diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 32c81b4630dc6a..410e9c3439cbb9 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -806,7 +806,6 @@ struct GenTree #define GTF_FLD_INITCLASS 0x20000000 // GT_FIELD/GT_CLS_VAR -- field access requires preceding class/static init helper #define GTF_INX_RNGCHK 0x80000000 // GT_INDEX/GT_INDEX_ADDR -- the array reference should be range-checked. -#define GTF_INX_REFARR_LAYOUT 0x20000000 // GT_INDEX -- TODO: Delete, no longer necessary (https://github.com/dotnet/runtime/issues/32647) #define GTF_INX_STRING_LAYOUT 0x40000000 // GT_INDEX -- this uses the special string array layout #define GTF_IND_TGT_NOT_HEAP 0x80000000 // GT_IND -- the target is not on the heap @@ -4632,11 +4631,6 @@ struct GenTreeIndex : public GenTreeOp gtFlags |= GTF_INX_RNGCHK; } - if (type == TYP_REF) - { - gtFlags |= GTF_INX_REFARR_LAYOUT; - } - gtFlags |= GTF_EXCEPT | GTF_GLOB_REF; } #if DEBUGGABLE_GENTREE diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 6d1079e2275c82..2e807cf3ce6d24 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -9099,15 +9099,7 @@ GenTree* Compiler::impFixupStructReturnType(GenTree* op, CORINFO_CLASS_HANDLE re GenTree* op1 = op->AsObj()->Addr(); // We will fold away OBJ/ADDR - // except for OBJ/ADDR/INDEX - // as the array type influences the array element's offset - // Later in this method we change op->gtType to info.compRetNativeType - // This is not correct when op is a GT_INDEX as the starting offset - // for the array elements 'elemOffs' is different for an array of - // TYP_REF than an array of TYP_STRUCT (which simply wraps a TYP_REF) - // Also refer to the GTF_INX_REFARR_LAYOUT flag - // - if ((op1->gtOper == GT_ADDR) && (op1->AsOp()->gtOp1->gtOper != GT_INDEX)) + if (op1->gtOper == GT_ADDR) { // Change '*(&X)' to 'X' and see if we can do better op = op1->AsOp()->gtOp1; diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 2b56413f955dec..69561e45be36f8 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -5552,8 +5552,15 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) // This is an array index expression. tree->gtFlags |= GTF_IND_ARR_INDEX; - /* An indirection will cause a GPF if the address is null */ - tree->gtFlags |= GTF_EXCEPT; + // If there's a bounds check, the the indir won't fault. + if (bndsChk) + { + tree->gtFlags |= GTF_IND_NONFAULTING; + } + else + { + tree->gtFlags |= GTF_EXCEPT; + } if (nCSE) { From 68b5e299c8772d80110e381ab4adccb5a506b92b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 4 Mar 2020 09:42:56 -0800 Subject: [PATCH 2/6] restore importer restriction on return tree --- src/coreclr/src/jit/importer.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 2e807cf3ce6d24..d90431245c77a9 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -9098,8 +9098,13 @@ GenTree* Compiler::impFixupStructReturnType(GenTree* op, CORINFO_CLASS_HANDLE re { GenTree* op1 = op->AsObj()->Addr(); - // We will fold away OBJ/ADDR - if (op1->gtOper == GT_ADDR) + // We will fold away OBJ/ADDR, except for OBJ/ADDR/INDEX + // + // In the latter case the OBJ type may have a different type + // than the array element type, and we need to preserve the + // array element type for now. + // + if ((op1->gtOper == GT_ADDR) && (op1->AsOp()->gtOp1->gtOper != GT_INDEX)) { // Change '*(&X)' to 'X' and see if we can do better op = op1->AsOp()->gtOp1; From 84786195533940933700a6592c2c9b94119d814f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 5 Mar 2020 13:11:45 -0800 Subject: [PATCH 3/6] don't annotate commas with field sequences --- src/coreclr/src/jit/morph.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 69561e45be36f8..2756f0f09ef429 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -17224,6 +17224,10 @@ void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZ bool fieldSeqRecorded = false; bool isMapAnnotation = false; + // Tunnel through any commas. + const bool commaOnly = true; + fieldSeqZero = fieldSeqZero->gtEffectiveVal(commaOnly); + #ifdef DEBUG if (verbose) { From f5a53717baecc8dc15a34dd6b30e44ee3b268485 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 5 Mar 2020 15:23:03 -0800 Subject: [PATCH 4/6] fix my fix --- src/coreclr/src/jit/morph.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 2756f0f09ef429..7abf26105d7e54 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -17219,15 +17219,18 @@ void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZ // We expect 'addr' to be an address at this point. assert(addr->TypeGet() == TYP_BYREF || addr->TypeGet() == TYP_I_IMPL || addr->TypeGet() == TYP_REF); + // Tunnel through any commas. + const bool commaOnly = true; + addr = addr->gtEffectiveVal(commaOnly); + + // We still expect 'addr' to be an address at this point. + assert(addr->TypeGet() == TYP_BYREF || addr->TypeGet() == TYP_I_IMPL || addr->TypeGet() == TYP_REF); + FieldSeqNode* fieldSeqUpdate = fieldSeqZero; GenTree* fieldSeqNode = addr; bool fieldSeqRecorded = false; bool isMapAnnotation = false; - // Tunnel through any commas. - const bool commaOnly = true; - fieldSeqZero = fieldSeqZero->gtEffectiveVal(commaOnly); - #ifdef DEBUG if (verbose) { From 682ac9f661dc544ac9f111f7980d96ec98836bb3 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 5 Mar 2020 18:30:26 -0800 Subject: [PATCH 5/6] more fixes --- src/coreclr/src/jit/optcse.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index 4a9a40541304c6..f8ecd24bd4af0e 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -2696,7 +2696,9 @@ class CSE_Heuristic GenTree* cse = nullptr; bool isDef; FieldSeqNode* fldSeq = nullptr; - bool hasZeroMapAnnotation = m_pCompiler->GetZeroOffsetFieldMap()->Lookup(exp, &fldSeq); + bool commaOnly = true; + GenTree* effectiveExp = exp->gtEffectiveVal(commaOnly); + const bool hasZeroMapAnnotation = m_pCompiler->GetZeroOffsetFieldMap()->Lookup(effectiveExp, &fldSeq); if (IS_CSE_USE(exp->gtCSEnum)) { @@ -2907,12 +2909,6 @@ class CSE_Heuristic // Assign the ssa num for the ref use. Note it may be the reserved num. ref->AsLclVarCommon()->SetSsaNum(cseSsaNum); - // If it has a zero-offset field seq, copy annotation to the ref - if (hasZeroMapAnnotation) - { - m_pCompiler->fgAddFieldSeqForZeroOffset(ref, fldSeq); - } - /* Create a comma node for the CSE assignment */ cse = m_pCompiler->gtNewOperNode(GT_COMMA, expTyp, origAsg, ref); cse->gtVNPair = ref->gtVNPair; // The comma's value is the same as 'val' From ad0588b6426a62883973af2d9c5ff3247c3b72bf Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 6 Mar 2020 18:07:06 -0800 Subject: [PATCH 6/6] fix some comment grammar --- src/coreclr/src/jit/morph.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 7abf26105d7e54..d97b0955ca35fb 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -5314,8 +5314,8 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) } #endif // FEATURE_SIMD - // Set up the the array length's offset into lenOffs - // And the the first element's offset into elemOffs + // Set up the array length's offset into lenOffs + // And the first element's offset into elemOffs ssize_t lenOffs; ssize_t elemOffs; if (tree->gtFlags & GTF_INX_STRING_LAYOUT) @@ -5552,7 +5552,7 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) // This is an array index expression. tree->gtFlags |= GTF_IND_ARR_INDEX; - // If there's a bounds check, the the indir won't fault. + // If there's a bounds check, the indir won't fault. if (bndsChk) { tree->gtFlags |= GTF_IND_NONFAULTING; @@ -9217,7 +9217,7 @@ GenTree* Compiler::fgMorphPromoteLocalInitBlock(GenTreeLclVar* destLclNode, GenT unsigned fieldLclNum = destLclVar->lvFieldLclStart + i; LclVarDsc* fieldDesc = lvaGetDesc(fieldLclNum); GenTree* dest = gtNewLclvNode(fieldLclNum, fieldDesc->TypeGet()); - // If it had been labeled a "USEASG", assignments to the the individual promoted fields are not. + // If it had been labeled a "USEASG", assignments to the individual promoted fields are not. dest->gtFlags |= (destLclNode->gtFlags & ~(GTF_NODE_MASK | GTF_VAR_USEASG)); GenTree* src; @@ -10217,7 +10217,7 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree) noway_assert(destLclNum != BAD_VAR_NUM); unsigned dstFieldLclNum = lvaTable[destLclNum].lvFieldLclStart + i; dstFld = gtNewLclvNode(dstFieldLclNum, lvaTable[dstFieldLclNum].TypeGet()); - // If it had been labeled a "USEASG", assignments to the the individual promoted fields are not. + // If it had been labeled a "USEASG", assignments to the individual promoted fields are not. if (destAddr != nullptr) { noway_assert(destAddr->AsOp()->gtOp1->gtOper == GT_LCL_VAR); @@ -13127,7 +13127,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // all the comma nodes the type of op1. // TODO: the comma flag update below is conservative and can be improved. // For example, if we made the ADDR(IND(x)) == x transformation, we may be able to - // get rid of some of the the IND flags on the COMMA nodes (e.g., GTF_GLOB_REF). + // get rid of some of the IND flags on the COMMA nodes (e.g., GTF_GLOB_REF). while (!commas.Empty()) {