Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import cgt.un(op, 0) as NE(op, 0) #54539

Merged
merged 3 commits into from
Jul 13, 2021
Merged

Import cgt.un(op, 0) as NE(op, 0) #54539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f598f0
Some minor code modernization
SingleAccretion Jun 22, 2021
d2dd580
Normalize GT_UN(op, 0) early in importer
SingleAccretion Jun 22, 2021
3b61c12
Solve most of the regressions
SingleAccretion Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13064,27 +13064,36 @@ 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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please check the case where op1 is zero (reversed) here - does it produce any diffs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's necessary: cgt.un(0, op) is always false, and op1 = gtFoldExpr(op1) below takes care of that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SingleAccretion no, I meant clt.un(0, op) the same pattern but reversed. Also, cle.un(op, 0) -- the same transformation in morph does that https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/morph.cpp#L13917-L13918

Copy link
Contributor Author

@SingleAccretion SingleAccretion Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for the clarification.

So I did add the clt.un(0, op) handling here, and the diffs were in about 10 methods across all collections, mostly positive as one would expect (amounting to ~150 bytes), but there was was one regression that got me thinking a bit. The regression is because GT_UN(x, 0) (reversed from LT(0, x)) allows assertion prop to eliminate a range check against zero, while NE(x, 0) doesn't have the same effect. Perhaps we should not be normalizing this idiom after all...

I will say that if it is ok, I would prefer to leave the case here as is. The long-term plan (well, plan for the PR after the next one) is to just fix the ordering problem in morph.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried exactly the same opt (to recognize GT_NE early in the importer) a long time ago but there were lots of range-checks regressions and I gave up so I guess those were fixed since then (by you? 🙂)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By @nathan-moore's #40180 would be my guess :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that should handle ==,!= 0 in range check. It's wierd that you saw regressions though since it got changed to != in morph before anything that should remove bound checks ran. ¯\_(ツ)_/¯

{
oper = GT_NE;
uns = false;
}

#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 */
if (opcode == CEE_CGT_UN || opcode == CEE_CLT_UN)
// TODO: setting both flags when only one is appropriate.
if (uns)
{
op1->gtFlags |= GTF_RELOP_NAN_UN | GTF_UNSIGNED;
}
Expand Down
64 changes: 37 additions & 27 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down