Skip to content

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

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

Merged
kunalspathak merged 3 commits into from
Jul 13, 2021
Merged

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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Normalize GT_UN(op, 0) early in importer 
Normalizing this idiom helps downstream optimizations.
  • Loading branch information
@SingleAccretion
SingleAccretion committed Jun 22, 2021
commit d2dd5800e5c7426968fe0ed93fcf88d836880174
10 changes: 9 additions & 1 deletion src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

@EgorBo EgorBo Jun 22, 2021

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

@SingleAccretion SingleAccretion Jun 23, 2021

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

@EgorBo EgorBo Jun 23, 2021

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

@EgorBo EgorBo Jun 23, 2021

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

@SingleAccretion SingleAccretion Jun 23, 2021

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

@nathan-moore nathan-moore Jun 23, 2021

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
// TODO-Casts: create a helper that upcasts int32 -> native int when necessary.
// See also identical code in impGetByRefResultType and STSFLD import.
Expand All @@ -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;
}
Expand Down