Skip to content

Clean up GT_NOP#95353

Merged
EgorBo merged 12 commits intodotnet:mainfrom
EgorBo:cleanup-gt-nop
Nov 30, 2023
Merged

Clean up GT_NOP#95353
EgorBo merged 12 commits intodotnet:mainfrom
EgorBo:cleanup-gt-nop

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 28, 2023

This PR addresses SingleAccretion and Jakob's feedback in #95249 (last comment)

So we now validate that GT_NOP has no child nodes and is always of TYP_VOID type

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 28, 2023
@ghost ghost assigned EgorBo Nov 28, 2023
@ghost
Copy link

ghost commented Nov 28, 2023

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

Issue Details

This PR addresses Jakob's feedback in #95249 (last comment)

So we now validate that GT_NOP has no child nodes and is always of TYP_VOID type

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

You can also remove the GTK_UNOP from gtlist.h.

return tree;
}

if (tree->IsNothingNode())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually reachable? Presumably, NOPs should go through fgMorphLeaf now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, gtFoldExpr called above may return a NOP (to be precise, gtFoldExprConst does that for:

*  BOUNDS_CHECK_Rng void   <l:$285, c:$284>
+--*  CNS_INT   int    0 $40
\--*  CNS_INT   int    1 $41

EgorBo and others added 5 commits November 29, 2023 00:28
@EgorBo
Copy link
Member Author

EgorBo commented Nov 29, 2023

/azp list

@EgorBo EgorBo marked this pull request as ready for review November 29, 2023 11:53
@azure-pipelines

This comment was marked as resolved.

@EgorBo
Copy link
Member Author

EgorBo commented Nov 29, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Nov 29, 2023

@jakobbotsch @dotnet/jit-contrib PTAL, this is ready for review, diffs

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, nice cleanup. Why is there a diff?

@EgorBo
Copy link
Member Author

EgorBo commented Nov 30, 2023

LGTM, nice cleanup. Why is there a diff?

It seems to only 1 method slightly regressed, but that method is quite huge (5k bytes of codegen).
I was able to obtain JitDump for before/after and the only difference I see is in the VN phase, so the change happend because we no longer number NOP, and it changed VN for the parents. So I presume it slightly changed CSE behaviour due to different (or same?) numbers. But since it's just 1 method I guess it's not worth investigating much.

image

@EgorBo EgorBo merged commit 3805c17 into dotnet:main Nov 30, 2023
@EgorBo EgorBo deleted the cleanup-gt-nop branch November 30, 2023 11:04
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants