KEMBAR78
JIT: Remove questionable transformation by EgorBo · Pull Request #95249 · dotnet/runtime · GitHub
Skip to content

JIT: Remove questionable transformation #95249

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
Merged
merged 2 commits into from
Nov 28, 2023
Merged

JIT: Remove questionable transformation #95249

merged 2 commits into from
Nov 28, 2023

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 26, 2023

No description provided.

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

ghost commented Nov 26, 2023

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

Issue Details

null

Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr
Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Nov 26, 2023

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch
jakobbotsch reviewed Nov 26, 2023
View reviewed changes
src/coreclr/jit/optimizer.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

What IR does this create for the example case we were looking at?

Copy link
Member Author

Choose a reason for hiding this comment

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

It used to be:

    [000002] ----G+-----                         *  CAST      long <- int
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa2213f21a static Fseq[s_13]

to

    [000002] ----G+-----                         *  NOP       byte  
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa2213f21a static Fseq[s_13]

Now it's:

    [000002] ----G+-----                         *  CAST      long <- int
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa22c7f21a static Fseq[s_13]

to

    [000002] ----G+-----                         *  CAST      int <- byte <- int
    [000001] n---G+-----                         \--*  IND       byte  
    [000000] H----+-----                            \--*  CNS_INT(h) long   0x7ffa22c7f21a static Fseq[s_13]

Copy link
Member

Choose a reason for hiding this comment

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

That's still an illegal transformation in this case since the value was an argument. The new cast does not guarantee widening into the upper 32 bits like the original cast does.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jakobbotsch are you suggesting to remove the whole transformation?

Copy link
Member

Choose a reason for hiding this comment

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

It seems there are two separate questions:

  1. Who is invoking optNarrowTree in this way that changes semantics of the program? The transformation is questionable to me but it's because the end result is a change change in behavior -- and this PR still has that problem.
  2. Can we get rid of the weird passthrough GT_NOP? Yes, and this PR probably accomplishes that, but it seems a better way would be to refactor optNarrowTree to take a GenTree** so that it can just replace the use of the cast with the operand in this situation.

I'm more worried about (1) than (2), though it would also be nice to clean up (2).

@build-analysis build-analysis bot mentioned this pull request Nov 26, 2023
Closed
@EgorBo EgorBo marked this pull request as ready for review November 27, 2023 10:07
@SingleAccretion
Copy link
Contributor

If you are deleting this quirky NOP variant, you can also delete code here:

runtime/src/coreclr/jit/valuenum.cpp

Lines 11371 to 11375 in 721c445

if (tree->OperIs(GT_NOP))
{
// Pass through arg vn.
tree->gtVNPair = tree->AsOp()->gtOp1->gtVNPair;
}

and case GT_NOP (as well as adjust the comment) here:

runtime/src/coreclr/jit/rationalize.cpp

Lines 252 to 264 in 721c445

case GT_NOP:
case GT_BOX:
case GT_ARR_ADDR:
// "optNarrowTree" sometimes inserts NOP nodes between defs and uses.
// In this case, remove the NOP. BOX/ARR_ADDR are such "passthrough"
// nodes by design, and at this point we no longer need them.
if (node->gtGetOp1() != nullptr)
{
use.ReplaceWith(node->gtGetOp1());
BlockRange().Remove(node);
node = node->gtGetOp1();
}
break;

@jakobbotsch jakobbotsch mentioned this pull request Nov 27, 2023
Merged
@EgorBo EgorBo force-pushed the remove-questionable-transform branch from 5ad1f2f to bbd96d7 Compare November 28, 2023 01:09
This was referenced Nov 28, 2023
Open
Open
@jakobbotsch jakobbotsch linked an issue Nov 28, 2023 that may be closed by this pull request
JIT: Assertion failed '!"Unexpected small type in IR"' #95318
Closed
@EgorBo
Copy link
Member Author

EgorBo commented Nov 28, 2023

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Nov 28, 2023
Closed
jakobbotsch
jakobbotsch approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There's probably some more stuff to fix around optNarrowTree, but this looks good to me to unblock the Fuzzlyn runs.

gtEffectiveVal can be cleaned up too now if passthrough NOPs no longer exist... but doesn't necessarily have to be in this PR.

@EgorBo EgorBo merged commit 0a5e211 into dotnet:main Nov 28, 2023
@EgorBo EgorBo deleted the remove-questionable-transform branch November 28, 2023 18:47
@jakobbotsch jakobbotsch mentioned this pull request Nov 28, 2023
Merged
@EgorBo EgorBo mentioned this pull request Nov 28, 2023
Merged
@jakobbotsch jakobbotsch mentioned this pull request Nov 29, 2023
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@EgorBo EgorBo

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JIT: Assertion failed '!"Unexpected small type in IR"'

3 participants

@EgorBo @SingleAccretion @jakobbotsch