Skip to content
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.

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

[JIT] X64/ARM64 - Fold 'x & 255' and 'x & 65535' to a cast #79630

Merged
merged 9 commits into from
Jan 5, 2023

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Dec 14, 2022

Description

Fixes #8833

Acceptance Criteria

  • Disasm tests

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 14, 2022
@ghost ghost assigned TIHan Dec 14, 2022
@ghost
Copy link

ghost commented Dec 14, 2022

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

Issue Details

Description

Fixes #8833

Acceptance Criteria

  • Disasm tests
Author: TIHan
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@TIHan TIHan marked this pull request as ready for review December 14, 2022 17:15
@TIHan TIHan changed the title [JIT] X64/ARM64 - Fold 'x & 256' and 'x & 65535' to a cast [JIT] X64/ARM64 - Fold 'x & 255' and 'x & 65535' to a cast Dec 14, 2022
@TIHan
Copy link
Contributor Author

TIHan commented Jan 4, 2023

@dotnet/jit-contrib @EgorBo This is ready

@EgorBo
Copy link
Member

EgorBo commented Jan 4, 2023

@TIHan the diffs seem to be expired - do you have the most recent ones maybe?

@TIHan TIHan closed this Jan 4, 2023
@TIHan TIHan reopened this Jan 4, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Jan 4, 2023

@EgorBo I will re-run CI to get new diffs.

assert(varTypeIsUnsigned(castToType));

GenTreeCast* cast = gtNewCastNode(TYP_INT, op1, false, castToType);
if (fgGlobalMorph)
Copy link
Member

Choose a reason for hiding this comment

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

can you educate me on why fgMorphTreeDone is needed here (I don't see it in other morph cases) and does it decrease jit-diffs if you just do this opt in GlobalMorph case only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following the precedent that was set. Right above NewZeroExtendNode, you see:

    // Helper function that creates a new IntCon node and morphs it, if required
    auto NewMorphedIntConNode = [&](int value) -> GenTreeIntCon* {
        GenTreeIntCon* icon = gtNewIconNode(value);
        if (fgGlobalMorph)
        {
            fgMorphTreeDone(icon);
        }
        return icon;
    };

Also, gtFoldExprSpecial has a comment that says:

// Return value:
//   Tree (possibly modified at root or below), or a new tree
//   Any new tree is fully morphed, if necessary

src/coreclr/jit/gentree.cpp Outdated Show resolved Hide resolved
@TIHan
Copy link
Contributor Author

TIHan commented Jan 5, 2023

@EgorBo Here are the diffs

@TIHan
Copy link
Contributor Author

TIHan commented Jan 5, 2023

Merging, the failed CI for Mono_LLVMFullAot is unrelated to this change.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 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.

Unnecessary extra and instruction in uint % 256
4 participants