Skip to content

JIT: Enable CSE for CLS/STR const handles#70580

Merged
EgorBo merged 6 commits into
dotnet:mainfrom
EgorBo:cse-handles
Jun 13, 2022
Merged

JIT: Enable CSE for CLS/STR const handles#70580
EgorBo merged 6 commits into
dotnet:mainfrom
EgorBo:cse-handles

Conversation

@EgorBo

@EgorBo EgorBo commented Jun 10, 2022

Copy link
Copy Markdown
Member

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)

@ghost ghost 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

ghost commented Jun 10, 2022

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@EgorBo

EgorBo commented Jun 10, 2022

Copy link
Copy Markdown
Member Author

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

EgorBo commented Jun 10, 2022

Copy link
Copy Markdown
Member Author

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

@kunalspathak

Copy link
Copy Markdown
Contributor

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

Ah, so the real benefit is on x64?

@EgorBo

EgorBo commented Jun 10, 2022

Copy link
Copy Markdown
Member Author

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

EgorBo commented Jun 11, 2022

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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
Copy Markdown
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

@jakobbotsch jakobbotsch left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

EgorBo commented Jun 13, 2022

Copy link
Copy Markdown
Member Author

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

EgorBo commented Jun 21, 2022

Copy link
Copy Markdown
Member Author

@EgorBo
EgorBo deleted the cse-handles branch June 21, 2022 17:25
@EgorBo

EgorBo commented Jun 28, 2022

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Contributor

@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

EgorBo commented Mar 20, 2024

Copy link
Copy Markdown
Member Author

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