Skip to content

JIT: Improve constant folding for commutative operators #656

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

Jump to bottom
Closed
EgorBo wants to merge 10 commits into from
Closed

JIT: Improve constant folding for commutative operators #656

EgorBo wants to merge 10 commits into from

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Dec 7, 2019 •
edited
Loading

Moved from dotnet/coreclr: dotnet/coreclr#27325

This PR re-uses existing GT_ADD constant folding logic for other commutative operators, e.g.:

static int Test1(int x) => x | 5 | 3; // quite popular case with Enums
static int Test2(int x) => x & 5 & 3;
static int Test3(int x) => x ^ 5 ^ 3;

static int Test4(int x, int y) => (x | 5) | (y | 3);
static int Test5(int x, int y) => (x & 5) & (y & 3);
static int Test6(int x, int y) => (x ^ 5) ^ (y ^ 3);

Diff for the snippet above: https://www.diffchecker.com/1Us280l1
Jit-utills Diff: https://gist.github.com/EgorBo/1238cbde8226194c554b4de83a900609

cc @dotnet/jit-contrib

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 7, 2019
sandreenko
sandreenko reviewed Dec 9, 2019
View reviewed changes
Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

The idea looks good, I have a few questions about the pre-existing logic here, that could lead to new errors if you apply it more broadly.

Why did you decide to delete the tests?

src/coreclr/src/jit/morph.cpp Outdated
{
// Don't create a byref pointer that may point outside of the ref object.
// If a GC happens, the byref won't get updated. This can happen if one
// of the int components is negative. It also requires the address generation
Copy link
Contributor

@sandreenko sandreenko Dec 9, 2019

Choose a reason for hiding this comment

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

// It also requires the address generation be in a fully-interruptible code region.

That comment looks incorrect, at least I do not see where we check that a fully-interruptible code region condition.

It is also not clear for me in which case that optimization for varTypeIsGC is incorrect, could somebody explain that to me please?

@EgorBo
Copy link
Member Author

EgorBo commented Dec 26, 2019 •
edited
Loading

@sandreenko thanks for feedback, will try to address it soon, sorry for the delay.

@EgorBo
Copy link
Member Author

EgorBo commented Jan 12, 2020

@sandreenko could you please re-review? thanks

sandreenko
sandreenko reviewed Jan 13, 2020
View reviewed changes
Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @EgorBo.
The checks for each case are not obvious, could you please explain them to me?

src/coreclr/src/jit/morph.cpp
// If a GC happens, the byref won't get updated. This can happen if one
// of the int components is negative. It also requires the address generation
// be in a fully-interruptible code region.
if (!varTypeIsGC(op1->AsOp()->gtGetOp1()->TypeGet()) && !varTypeIsGC(op2->AsOp()->gtGetOp1()->TypeGet()))
Copy link
Contributor

@sandreenko sandreenko Jan 13, 2020

Choose a reason for hiding this comment

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

here you check varTypeIsGC(x), varTypeIsGC(y), gtIsActiveCSE_Candidate(op2).

Copy link
Member Author

@EgorBo EgorBo Jan 16, 2020

Choose a reason for hiding this comment

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

Added also gtIsActiveCSE_Candidate(op1) here

src/coreclr/src/jit/morph.cpp
if (op2->IsCnsIntOrI() && varTypeIsIntegralOrI(tree->TypeGet()))
{
// Fold "((x <op> icon1) <op> icon2) to (x <op> (icon1 <op> icon2))"
if (op1->OperIs(oper) && !gtIsActiveCSE_Candidate(op1) && op1->AsOp()->gtGetOp2()->IsCnsIntOrI() &&
Copy link
Contributor

@sandreenko sandreenko Jan 13, 2020

Choose a reason for hiding this comment

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

here you check varTypeIsGC(icon1)? why does it need that check? Why don't you need to check x here as you do in the previous case?
You check gtIsActiveCSE_Candidate(op1), in the previous case you were checking gtIsActiveCSE_Candidate(op2), what is the difference?

Copy link
Member Author

@EgorBo EgorBo Jan 16, 2020

Choose a reason for hiding this comment

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

removed varTypeIsGC(icon1) and added gtIsActiveCSE_Candidate(op1) to the first expression

src/coreclr/src/jit/morph.cpp
const ssize_t icon1 = cns1->IconValue();
const ssize_t icon2 = cns2->IconValue();

if (oper == GT_ADD)
Copy link
Contributor

@sandreenko sandreenko Jan 13, 2020

Choose a reason for hiding this comment

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

that code duplication doesn't look cool, but a function with 6 arguments GenTreeIntConCommon* fgGetCommutativeResult(oper, icon1, icon2, tree, op1, optional op2) also doesn't. I am not sure here.

Copy link
Member Author

@EgorBo EgorBo Jan 16, 2020

Choose a reason for hiding this comment

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

Neither am I, tried to simplify it a bit. Let me know if I should extract it to a separate method.

@sandreenko
Copy link
Contributor

sandreenko commented Jan 16, 2020

Please take a look at the new test failures after the last change that I have not seen before, like x86 windows:

JIT\\HardwareIntrinsics\\X86\\Avx\\ConvertToVector_ro\\ConvertToVector_ro.cmd

Assert failure(PID 6060 [0x000017ac], Thread: 5392 [0x1510]): Assertion failed 'type != TYP_REF || val == 0' in 'JIT.HardwareIntrinsics.X86.SimpleUnaryOpTest__ConvertToVector128Int32WithTruncationDouble:ValidateResult(System.Double[],System.Int32[],System.String):this' (IL size 165)

    File: F:\workspace\_work\1\s\src\coreclr\src\jit\codegenxarch.cpp Line: 39
    Image: C:\h\w\AC710973\p\CoreRun.exe

@sandreenko sandreenko mentioned this pull request Feb 13, 2020
Merged
@stephentoub
Copy link
Member

stephentoub commented Feb 17, 2020

@EgorBo, still working on this? Thanks.

@stephentoub
Copy link
Member

stephentoub commented Mar 13, 2020

@EgorBo, seems like you're not working on this anymore. Shall we close it?

@EgorBo
Copy link
Member Author

EgorBo commented Mar 16, 2020

Sorry, will re-open it later with proper fixes 🙁

@EgorBo EgorBo closed this Mar 16, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@sandreenko sandreenko sandreenko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI optimization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@EgorBo @sandreenko @stephentoub @Dotnet-GitSync-Bot