Skip to content

JIT: Improve constant folding for commutative operators - #656

Closed
EgorBo wants to merge 10 commits into
dotnet:masterfrom
EgorBo:improve-cfold
Closed

JIT: Improve constant folding for commutative operators#656
EgorBo wants to merge 10 commits into
dotnet:masterfrom
EgorBo:improve-cfold

Conversation

@EgorBo

@EgorBoEgorBo commented Dec 7, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Moved from dotnet/coreclr: dotnet/coreclr#27325

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

staticintTest1(intx)=>x|5|3;// quite popular case with EnumsstaticintTest2(intx)=>x&5&3;staticintTest3(intx)=>x^5^3;staticintTest4(intx,inty)=>(x|5)|(y|3);staticintTest5(intx,inty)=>(x&5)&(y&3);staticintTest6(intx,inty)=>(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-BotDotnet-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

@sandreenkosandreenko left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadsrc/coreclr/src/jit/morph.cpp
Comment threadsrc/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

@sandreenkosandreenkoDec 9, 2019

Copy link
Copy Markdown
Contributor

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?

Comment threadsrc/coreclr/src/jit/morph.cpp Outdated
Comment threadsrc/coreclr/src/jit/morph.cpp Outdated
@EgorBo

EgorBo commented Dec 26, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

@EgorBo

EgorBo commented Jan 12, 2020

Copy link
Copy Markdown
MemberAuthor

@sandreenko could you please re-review? thanks

sandreenko
sandreenko reviewed Jan 13, 2020
View reviewed changes

@sandreenkosandreenko left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadsrc/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()))

@sandreenkosandreenkoJan 13, 2020

Copy link
Copy Markdown
Contributor

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).

@EgorBoEgorBoJan 16, 2020

Copy link
Copy Markdown
MemberAuthor

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

Comment threadsrc/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() &&

@sandreenkosandreenkoJan 13, 2020

Copy link
Copy Markdown
Contributor

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?

@EgorBoEgorBoJan 16, 2020

Copy link
Copy Markdown
MemberAuthor

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

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

if (oper == GT_ADD)

@sandreenkosandreenkoJan 13, 2020

Copy link
Copy Markdown
Contributor

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.

@EgorBoEgorBoJan 16, 2020

Copy link
Copy Markdown
MemberAuthor

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

sandreenko commented Jan 16, 2020

Copy link
Copy Markdown
Contributor

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

@sandreenkosandreenko mentioned this pull request Feb 13, 2020
Merged
@stephentoub

stephentoub commented Feb 17, 2020

Copy link
Copy Markdown
Member

@EgorBo, still working on this? Thanks.

@stephentoub

stephentoub commented Mar 13, 2020

Copy link
Copy Markdown
Member

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

@EgorBo

EgorBo commented Mar 16, 2020

Copy link
Copy Markdown
MemberAuthor

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

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

Reviewers

1 more reviewer
@sandreenkosandreenkosandreenko left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIoptimization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

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