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: Enable CSE for CLS/STR const handles #70580

Merged
merged 6 commits into from
Jun 13, 2022
Merged

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 10, 2022

Motivated by #59704

Example:

public static void Test(object left, object right)
{
    if (left.GetType() == typeof(string) ||
        right.GetType() == typeof(string))
    {
        Console.WriteLine();
    }
}

codegen diff: https://www.diffchecker.com/tI9sCqxx

Quite impactful Diffs (up to -881922 on x64 and -2283090 on arm32)

@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 Jun 10, 2022
@ghost ghost assigned EgorBo Jun 10, 2022
@ghost
Copy link

ghost commented Jun 10, 2022

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

Issue Details

Motivated by #59704

Example:

public static void Test(object left, object right)
{
    if (left.GetType() == typeof(string) ||
        right.GetType() == typeof(string))
    {
        Console.WriteLine();
    }
}

codegen diff: https://www.diffchecker.com/tI9sCqxx

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member

This is more impactful on arm64 because of 4 instructions we have to produce the constant.

@EgorBo
Copy link
Member Author

EgorBo commented Jun 10, 2022

This is more impactful on arm64 because of 4 instructions we have to produce the constant.

On arm64 it works as is - CSE for constants is enabled, cost is adjusted (e.g. [4, 12] as far as I remember)

@EgorBo
Copy link
Member Author

EgorBo commented Jun 10, 2022

asm.libraries.pmi.windows.x64.checked is promising - -48955 (-0.10 % of base)

@kunalspathak
Copy link
Member

On arm64 it works as is - CSE for constants is enabled

Ah, so the real benefit is on x64?

@EgorBo
Copy link
Member Author

EgorBo commented Jun 10, 2022

On arm64 it works as is - CSE for constants is enabled

Ah, so the real benefit is on x64?

yep, I expect zero diffs on arms

@EgorBo EgorBo marked this pull request as ready for review June 11, 2022 11:25
@EgorBo
Copy link
Member Author

EgorBo commented Jun 11, 2022

@dotnet/jit-contrib PTAL, diffs
There are two improvements here, 1) it enables CSE of constants for ARM32 (it was only enabled for ARM64) - it leads to up to

Total bytes of delta: -2283090 (-1.86 % of base)

on arm32
2) It enables CSE of some specific constant handles on x64 (I was mainly motivated by PGO where we emit CLASS handles a lot) and this perf regression - #59704

Total bytes of delta: -72683 (-0.14 % of base)

for libraries.pmi.windows.x64.checked.mch

There are size/PerfScore regressions but it's impossible to tune CSE and not getting them, regressions are mainly due to register pressure - more spills/restores, etc.

No diffs on arm64 as expected

Comment on lines +791 to +793
// Unconditionally allow these constant handles to be CSE'd
!tree->IsIconHandle(GTF_ICON_STATIC_HDL) && !tree->IsIconHandle(GTF_ICON_CLASS_HDL) &&
!tree->IsIconHandle(GTF_ICON_STR_HDL))
Copy link
Member

Choose a reason for hiding this comment

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

Why only these types of handles?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why only these types of handles?

when I enable all of them I see a lot of size/perfscore regression. I was only interested in GTF_ICON_CLASS_HDL (for PGO), String literals and statics were added because they produced good diffs as well

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, but I think you should run stress jobs.

if (tree->IsIntegralConst())
{
if (!enableConstCSE)
if (!enableConstCSE &&
Copy link
Contributor

Choose a reason for hiding this comment

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

It is unsatisfying that we will now CSE things even when explicitly asked not to (CONST_CSE_DISABLE_ALL).

Perhaps the easiest solution would be to delete CONST_CSE_DISABLE_ALL. Or check it separately (if (forceDisableConstCSE) continue;).

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I'll delete CONST_CSE_DISABLE_ALL in a follow up PR, thanks

@EgorBo EgorBo merged commit 76d3224 into dotnet:main Jun 13, 2022
@EgorBo
Copy link
Member Author

EgorBo commented Jun 13, 2022

LGTM, but I think you should run stress jobs.

oops didn't notice your suggestion, I assume it just partially enables what was already fully enabled for arm. I'll check if I can enable CSE for even more things under JitStress mode

@EgorBo
Copy link
Member Author

EgorBo commented Jun 21, 2022

@EgorBo EgorBo deleted the cse-handles branch June 21, 2022 17:25
@EgorBo
Copy link
Member Author

EgorBo commented Jun 28, 2022

@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2022
@BruceForstall
Copy link
Member

@EgorBo This PR enabled shared constant CSE for ARM (confusingly, the comments around JitConstCSE still only say ARM64).

But it didn't enable CSE of constants (non-shared) for ARM. Does that make sense? (It makes the JitConstCSE config comments even more confusing.) Should this ifdef be TARGET_ARMARCH?

#if !defined(TARGET_ARM64)

@EgorBo
Copy link
Member Author

EgorBo commented Mar 20, 2024

This PR enabled shared constant CSE for ARM

I don't remember the details, ARM32 wasn't initialy the goal of this PR, it was just enabled along the road (produced huge diffs), if nonshared CSE is not enabled - it should propably be enabled too I presume?

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.

5 participants