From 6f598f002f27104807a92ed885f8acad75c45b0d Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 22 Jun 2021 09:29:30 +0300 Subject: [PATCH 1/3] Some minor code modernization Use "genActualType" directly as it is now a templated method. Don't create casts to TYP_ULONG - they are identical to casts to TYP_LONG. TYP_ULONG is only relevant for checked casts. Add a TODO on addressing the duplicated logic that upcasts to native int from int32 on 64 bit. Use modern comments. Zero diffs. --- src/coreclr/jit/importer.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index d96773def7f1be..4b6a6ddb06bc02 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -13065,25 +13065,26 @@ void Compiler::impImportBlockCode(BasicBlock* block) op1 = impPopStack().val; #ifdef TARGET_64BIT - if (varTypeIsI(op1->TypeGet()) && (genActualType(op2->TypeGet()) == TYP_INT)) + // TODO-Casts: create a helper that upcasts int32 -> native int when necessary. + // See also identical code in impGetByRefResultType and STSFLD import. + if (varTypeIsI(op1) && (genActualType(op2) == TYP_INT)) { - op2 = gtNewCastNode(TYP_I_IMPL, op2, uns, uns ? TYP_U_IMPL : TYP_I_IMPL); + op2 = gtNewCastNode(TYP_I_IMPL, op2, uns, TYP_I_IMPL); } - else if (varTypeIsI(op2->TypeGet()) && (genActualType(op1->TypeGet()) == TYP_INT)) + else if (varTypeIsI(op2) && (genActualType(op1) == TYP_INT)) { - op1 = gtNewCastNode(TYP_I_IMPL, op1, uns, uns ? TYP_U_IMPL : TYP_I_IMPL); + op1 = gtNewCastNode(TYP_I_IMPL, op1, uns, TYP_I_IMPL); } #endif // TARGET_64BIT - assertImp(genActualType(op1->TypeGet()) == genActualType(op2->TypeGet()) || - (varTypeIsI(op1->TypeGet()) && varTypeIsI(op2->TypeGet())) || - (varTypeIsFloating(op1->gtType) && varTypeIsFloating(op2->gtType))); + assertImp(genActualType(op1) == genActualType(op2) || (varTypeIsI(op1) && varTypeIsI(op2)) || + (varTypeIsFloating(op1) && varTypeIsFloating(op2))); - /* Create the comparison node */ + // Create the comparison node. op1 = gtNewOperNode(oper, TYP_INT, op1, op2); - /* TODO: setting both flags when only one is appropriate */ + // TODO: setting both flags when only one is appropriate. if (opcode == CEE_CGT_UN || opcode == CEE_CLT_UN) { op1->gtFlags |= GTF_RELOP_NAN_UN | GTF_UNSIGNED; From d2dd5800e5c7426968fe0ed93fcf88d836880174 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 22 Jun 2021 10:18:44 +0300 Subject: [PATCH 2/3] Normalize GT_UN(op, 0) early in importer Normalizing this idiom helps downstream optimizations. --- src/coreclr/jit/importer.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 4b6a6ddb06bc02..3fc68769c2aff0 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -13064,6 +13064,14 @@ void Compiler::impImportBlockCode(BasicBlock* block) op2 = impPopStack().val; op1 = impPopStack().val; + // Recognize the IL idiom of CGT_UN(op1, 0) and normalize + // it so that downstream optimizations don't have to. + if ((opcode == CEE_CGT_UN) && op2->IsIntegralConst(0)) + { + oper = GT_NE; + uns = false; + } + #ifdef TARGET_64BIT // TODO-Casts: create a helper that upcasts int32 -> native int when necessary. // See also identical code in impGetByRefResultType and STSFLD import. @@ -13085,7 +13093,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) op1 = gtNewOperNode(oper, TYP_INT, op1, op2); // TODO: setting both flags when only one is appropriate. - if (opcode == CEE_CGT_UN || opcode == CEE_CLT_UN) + if (uns) { op1->gtFlags |= GTF_RELOP_NAN_UN | GTF_UNSIGNED; } From 3b61c12892cd19876f17cee1abbe734ec0852442 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 22 Jun 2021 11:40:59 +0300 Subject: [PATCH 3/3] Solve most of the regressions In morph, when narrowing the AND operand, only insert casts if necessary - prefer to use "optNarrowTree". Otherwise we end up with redundant register shuffling. --- src/coreclr/jit/morph.cpp | 64 ++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b6e1f808ea9094..78d99cdffd6474 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13789,44 +13789,54 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) noway_assert(op1->TypeGet() == TYP_LONG && op1->OperGet() == GT_AND); - /* Is the result of the mask effectively an INT ? */ - - GenTree* andMask; - andMask = op1->AsOp()->gtOp2; - if (andMask->gtOper != GT_CNS_NATIVELONG) - { - goto COMPARE; - } - if ((andMask->AsIntConCommon()->LngValue() >> 32) != 0) + // The transform below cannot preserve VNs. + if (fgGlobalMorph) { - goto COMPARE; - } + // Is the result of the mask effectively an INT ? - /* Now we know that we can cast AsOp()->gtOp1 of AND to int */ + GenTree* andMask = op1->AsOp()->gtOp2; + + if (andMask->gtOper != GT_CNS_NATIVELONG) + { + goto COMPARE; + } + if ((andMask->AsIntConCommon()->LngValue() >> 32) != 0) + { + goto COMPARE; + } - op1->AsOp()->gtOp1 = gtNewCastNode(TYP_INT, op1->AsOp()->gtOp1, false, TYP_INT); + // Now we narrow AsOp()->gtOp1 of AND to int. + if (optNarrowTree(op1->AsOp()->gtGetOp1(), TYP_LONG, TYP_INT, ValueNumPair(), false)) + { + optNarrowTree(op1->AsOp()->gtGetOp1(), TYP_LONG, TYP_INT, ValueNumPair(), true); + } + else + { + op1->AsOp()->gtOp1 = gtNewCastNode(TYP_INT, op1->AsOp()->gtGetOp1(), false, TYP_INT); + } - /* now replace the mask node (AsOp()->gtOp2 of AND node) */ + // now replace the mask node (AsOp()->gtOp2 of AND node). - noway_assert(andMask == op1->AsOp()->gtOp2); + noway_assert(andMask == op1->AsOp()->gtOp2); - ival1 = (int)andMask->AsIntConCommon()->LngValue(); - andMask->SetOper(GT_CNS_INT); - andMask->gtType = TYP_INT; - andMask->AsIntCon()->gtIconVal = ival1; + ival1 = (int)andMask->AsIntConCommon()->LngValue(); + andMask->SetOper(GT_CNS_INT); + andMask->gtType = TYP_INT; + andMask->AsIntCon()->gtIconVal = ival1; - /* now change the type of the AND node */ + // now change the type of the AND node. - op1->gtType = TYP_INT; + op1->gtType = TYP_INT; - /* finally we replace the comparand */ + // finally we replace the comparand. - ival2 = (int)cns2->AsIntConCommon()->LngValue(); - cns2->SetOper(GT_CNS_INT); - cns2->gtType = TYP_INT; + ival2 = (int)cns2->AsIntConCommon()->LngValue(); + cns2->SetOper(GT_CNS_INT); + cns2->gtType = TYP_INT; - noway_assert(cns2 == op2); - cns2->AsIntCon()->gtIconVal = ival2; + noway_assert(cns2 == op2); + cns2->AsIntCon()->gtIconVal = ival2; + } goto COMPARE;