Skip to content

Fix incorrect VN when folding GT_NEG(GT_MUL(A, C)) - #57651

Merged
jakobbotsch merged 2 commits into
dotnet:mainfrom
jakobbotsch:fix-57640
Aug 18, 2021
Merged

Fix incorrect VN when folding GT_NEG(GT_MUL(A, C))#57651
jakobbotsch merged 2 commits into
dotnet:mainfrom
jakobbotsch:fix-57640

Conversation

@jakobbotsch

@jakobbotschjakobbotsch commented Aug 18, 2021

Copy link
Copy Markdown
Member

Fixes#57640

cc @dotnet/jit-contrib @SingleAccretion

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 18, 2021
@ghost

ghost commented Aug 18, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #57640

cc @dotnet/jit-contrib @SingleAccretion

Author:jakobbotsch
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@EgorBo

EgorBo commented Aug 18, 2021

Copy link
Copy Markdown
Member

I guess it has to be back ported to 6.0 branch

@jakobbotsch

jakobbotsch commented Aug 18, 2021

Copy link
Copy Markdown
MemberAuthor

I guess it has to be back ported to 6.0 branch

Yep, I'll do it once this makes it through CI

SingleAccretion
SingleAccretion reviewed Aug 18, 2021
View reviewed changes
Comment threadsrc/coreclr/jit/morph.cpp
@@ -13177,6 +13177,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
GenTree* newOp2 = gtNewIconNode(-constVal, op1op2->TypeGet()); // -C

@SingleAccretionSingleAccretionAug 18, 2021

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.

I think we also want to update the VN for the new constant (this is what we do in other places). But since we haven't actually found a bug with it (yet), and the way to do it is kinda verbose right now, and I will be PRing changes that will make it less verbose soon, and the fact that this PR is likely to be backported, I think it is fine to leave this as is.

@jakobbotschjakobbotschAug 18, 2021

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.

Right, this ends up as a constant without any VN, but my impression was that we handle this ok downstream.

@SingleAccretionSingleAccretionAug 18, 2021

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.

I similarly haven't seen places in the optimizer that would trip up on a NoVN constant tree. I would not be surprised if they exist though, these contracts on what should and should not be maintained are very implicit.

@EgorBo

EgorBo commented Aug 18, 2021

Copy link
Copy Markdown
Member

@jakobbotsch can you please do the same with https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/morph.cpp#L11175-L11189 or ideally somehow unify it with NEG(MUL(A,C)) opt

@EgorBo

EgorBo commented Aug 18, 2021

Copy link
Copy Markdown
Member

and https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/morph.cpp#L11271-L11286 for div

@jakobbotsch

jakobbotsch commented Aug 18, 2021

Copy link
Copy Markdown
MemberAuthor

@EgorBo I don't think those have the problem since they don't return a sub tree.

@EgorBo

EgorBo commented Aug 18, 2021

Copy link
Copy Markdown
Member

@EgorBo I don't think those have the problem since they don't return a sub tree.

ah, right, nvm, in case of MUL(NEG(X), CNS) VN for MUL remains the same after opt correctly

echesakov
echesakov approved these changes Aug 18, 2021
View reviewed changes

@echesakovechesakov 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.

LGTM

@jakobbotsch

jakobbotsch commented Aug 18, 2021

Copy link
Copy Markdown
MemberAuthor

CI failures look like #57620

@jakobbotsch
jakobbotsch merged commit ec305c7 into dotnet:mainAug 18, 2021
@jakobbotsch
jakobbotsch deleted the fix-57640 branch August 18, 2021 20:04
@jakobbotsch

jakobbotsch commented Aug 18, 2021

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@github-actions

github-actionsBot commented Aug 18, 2021

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1144500550

@github-actionsgithub-actionsBot mentioned this pull request Aug 18, 2021
Merged
@JulieLeeMSFTJulieLeeMSFT added this to the 6.0.0 milestone Aug 18, 2021
@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBoEgorBoEgorBo approved these changes
+2 more reviewers
@SingleAccretionSingleAccretionSingleAccretion left review comments
@echesakovechesakovechesakov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jakobbotschjakobbotsch

Labels

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

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

JIT: Invalid negated result

5 participants

@jakobbotsch@EgorBo@echesakov@SingleAccretion@JulieLeeMSFT