Adding additional 8 eGPR. #113988

DeepakRajendrakumaran · 2025-03-27T21:24:54Z

Overview

This PR does the following 2 things

Adds an additional 8 eGPR taking the total number of x86 GPR to 32 and total registers to 72
Adds a way to skip unused registers if iterating through RegRecords.
on x64 without APX for instance, REG16-REG_31 are unused. We iterate through this processing them currently. This allows us to just skip these

TPDIFF impact

From CI for this PR after last few PRs to improve tpdiff(See here for numbers before)

windows x64

Overall (+1.67% to +2.54%)

Collection	PDIFF
aspnet.run.windows.x64.checked.mch	+2.07%
benchmarks.run.windows.x64.checked.mch	+1.67%
benchmarks.run_pgo.windows.x64.checked.mch	+1.83%
benchmarks.run_pgo_optrepeat.windows.x64.checked.mch	+1.67%
coreclr_tests.run.windows.x64.checked.mch	+2.54%
libraries.crossgen2.windows.x64.checked.mch	+2.18%
libraries.pmi.windows.x64.checked.mch	+1.82%
libraries_tests.run.windows.x64.Release.mch	+2.20%
libraries_tests_no_tiered_compilation.run.windows.x64.Release.mch	+1.72%
realworld.run.windows.x64.checked.mch	+1.71%
smoke_tests.nativeaot.windows.x64.checked.mch	+1.75%

MinOpts (+3.14% to +5.71%)

Collection	PDIFF
aspnet.run.windows.x64.checked.mch	+4.34%
benchmarks.run.windows.x64.checked.mch	+4.60%
benchmarks.run_pgo.windows.x64.checked.mch	+4.28%
benchmarks.run_pgo_optrepeat.windows.x64.checked.mch	+4.57%
coreclr_tests.run.windows.x64.checked.mch	+3.49%
libraries.crossgen2.windows.x64.checked.mch	+4.13%
libraries.pmi.windows.x64.checked.mch	+3.14%
libraries_tests.run.windows.x64.Release.mch	+4.40%
libraries_tests_no_tiered_compilation.run.windows.x64.Release.mch	+3.71%
realworld.run.windows.x64.checked.mch	+5.71%
smoke_tests.nativeaot.windows.x64.checked.mch	+5.01%

FullOpts (+1.63% to +2.18%)

Collection	PDIFF
aspnet.run.windows.x64.checked.mch	+1.68%
benchmarks.run.windows.x64.checked.mch	+1.67%
benchmarks.run_pgo.windows.x64.checked.mch	+1.66%
benchmarks.run_pgo_optrepeat.windows.x64.checked.mch	+1.67%
coreclr_tests.run.windows.x64.checked.mch	+1.84%
libraries.crossgen2.windows.x64.checked.mch	+2.18%
libraries.pmi.windows.x64.checked.mch	+1.82%
libraries_tests.run.windows.x64.Release.mch	+1.63%
libraries_tests_no_tiered_compilation.run.windows.x64.Release.mch	+1.68%
realworld.run.windows.x64.checked.mch	+1.70%
smoke_tests.nativeaot.windows.x64.checked.mch	+1.75%

Alternate test numbers from running superpmi directly and using the 'Total Time':

% Regression = ((Diff Mean - Base Mean)/ Base Mean) * 100)

This reverts commit cc6d454.

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (11)

src/coreclr/jit/compiler.cpp: Language not supported
src/coreclr/jit/emit.h: Language not supported
src/coreclr/jit/emitxarch.cpp: Language not supported
src/coreclr/jit/emitxarch.h: Language not supported
src/coreclr/jit/lsra.cpp: Language not supported
src/coreclr/jit/lsra.h: Language not supported
src/coreclr/jit/lsrabuild.cpp: Language not supported
src/coreclr/jit/register.h: Language not supported
src/coreclr/jit/target.h: Language not supported
src/coreclr/jit/targetamd64.h: Language not supported
src/coreclr/jit/unwindamd64.cpp: Language not supported

DeepakRajendrakumaran · 2025-04-21T14:50:00Z

@BruceForstall @jakobbotsch @kunalspathak This PR is ready for review

BruceForstall · 2025-04-21T16:27:58Z

Diffs

kunalspathak · 2025-04-21T16:38:59Z

Diffs

@DeepakRajendrakumaran - can you double check why TP of arm64 got affected?

DeepakRajendrakumaran · 2025-04-21T16:44:50Z

Diffs

@DeepakRajendrakumaran - can you double check why TP of arm64 got affected?

This commit affects all targets but should be negligible for non x64 - commit

This is to skip over eGPR for non APX x64 machines when iterating over RegisterRecord. This can be extended to arm64 if there are any registers needed to be skipped over with/without features for arm64. Will need to add something similar - Change for x86

DeepakRajendrakumaran · 2025-04-21T16:49:04Z

@kunalspathak This is how tpdiff looks with just the eGPR and without the logic to skip regs

BruceForstall

Some questions

BruceForstall · 2025-04-21T16:52:31Z

src/coreclr/jit/target.h

@@ -239,6 +239,10 @@ typedef uint64_t regMaskSmall;
 #define HAS_MORE_THAN_64_REGISTERS 1
 #endif // TARGET_ARM64

+#ifdef TARGET_AMD64


Merge with ARM64 case above?

BruceForstall · 2025-04-21T16:56:49Z

src/coreclr/jit/emit.h

@@ -791,8 +791,10 @@ class emitter
        // Note that we use the _idReg1 and _idReg2 fields to hold
        // the live gcrefReg mask for the call instructions on x86/x64
        //
+#if !defined(TARGET_AMD64)


Why did you move this only for x64?

The "Space taken up to here" comment, below, is now wrong and needs to be updated, as do the "Space taken up to here" comments that follow.

Why did you move this only for x64?

It's a bit of a weird problem. REGNUM_BITS changing from 6 to 7 results in _idReg1 and _idReg2 taking up an extra bit each. This throws off the instrDesc size for x64 and increases it significantly due to padding. My solution was to move around the position of _idReg1 and _idReg2 to a position for x64 which did not cause addition of as many padding bits.

I wonder why it matters where _idReg1/_idReg2 are placed to determine padding. All type types here have a base type of unsigned which means that bitfields of them should all be able bit packable. It might make sense for performance reasons to avoid splitting _idReg1/_idReg2 across 32-bit (or maybe even byte) boundaries, but that's not why you did it.

For reference

In main with no changes

If I just add new registers with no change in order for x64

With the change highlighted by you in the PR

If i leave it as was, _idReg1 cannot stay within 32 bit boundary since it starts at Byte 3, Bit 2(It was not a problem earlier when it was 6 bits). So, it gets moved to Byte 4, Bit 0

Edit - that's my interpretation

Oh, I see: a bit field can't extend beyond the limit of the base type (here, unsigned).

btw, is the tool your using to show field offsets public?

The screenshot I used here is from Visual Studio. Click on memory layout from below

The memory viewer in Visual Studio Code isn't as intuitive

I have updated the "Space taken up to here" comments

BruceForstall · 2025-04-21T17:04:33Z

src/coreclr/jit/lsrabuild.cpp

@@ -1936,6 +1936,10 @@ void LinearScan::buildPhysRegRecords()
        RegRecord* curr = &physRegs[reg];
        curr->init(reg);
    }
+#if defined(TARGET_AMD64)


Doesn't this fall out naturally? Or are there cases where REG_NEXT(get_REG_INT_LAST()) != REG_FP_FIRST?

Should this be all-platform (no ifdef)?

This is a x64 specific issue. There are 2 cases

x64 with APX ON
get_REG_INT_LAST() = REG_R31 and so REG_NEXT(get_REG_INT_LAST()) = REG_FP_FIRST

x64 with APX OFF
get_REG_INT_LAST() = REG_R15 and so REG_NEXT(get_REG_INT_LAST()) != REG_FP_FIRST
This is because REG_NEXT(get_REG_INT_LAST()) = REG_R16. REG_R16 is defined but not valid for x64 targets with no APX support

BruceForstall · 2025-04-21T17:06:58Z

src/coreclr/jit/emitxarch.h

    // TODO-apx: the definition here is incorrect, we will need to revisit this after we extend the register definition.
    //           for now, we can simply use REX2 as REX.


Is this comment still needed?

Have removed this comment

BruceForstall

This LGTM. Would like to hear @kunalspathak feedback on the LSRA changes.

kunalspathak

Added few questions/comments

kunalspathak · 2025-04-21T23:39:40Z

src/coreclr/jit/lsra.h

@@ -544,6 +545,7 @@ class RegRecord : public Referenceable
    Interval* previousInterval;

    regNumber     regNum;
+    regNumber     nextRegNum; // the next active register.


This is adding additional 288 bytes of memory per method compilation.

Did you take a stab at #112959 (comment) and see if that is cheap?

I did not try the regIndices method. I forgot about it. Is this what you had in mind?

Assuming you fix the ACTUAL_REG_COUNT in the else portion of isApxSupported, yes.

ACTUAL_REG_COUNT

I didn't change ACTUAL_REG_COUNT because I don't think that can be used as the condition as presently used.

The loops look like the following

for (regNumber reg = REG_FIRST; reg < AVAILABLE_REG_COUNT; reg = REG_NEXT(reg)) { .............. }

AVAILABLE_REG_COUNT can be either of following

ACTUAL_REG_COUNT

or

ACTUAL_REG_COUNT - (CNT_HIGHFLOAT + CNT_MASK_REGS)

I ended up not changing ACTUAL_REG_COUNT but rather modifying the loop

int index = 1; for (regNumber reg = (regNumber)regIndices[0]; reg < AVAILABLE_REG_COUNT; reg = (regNumber)regIndices[index++]) { ........................ }

This allows me to keep using AVAILABLE_REG_COUNT

Alternative would be to update ACTUAL_REG_COUNT and use it directly in the for loops

can you update it to be something like and that way you are not affecting non-xarch platform? Also, you don't need separate activeRegsCount in that case.

#ifdef TARGET_XARCH #define NEXT_REGISTER(i) i++, reg = regIndices[i] #else #define NEXT_REGISTER(reg) REG_NEXT(reg) #endif for (regNumber reg = REG_FIRST; reg < AVAILABLE_REG_COUNT; reg = NEXT_REGISTER(reg)) { ... }

That doesn't work as is. We will need to track an index and a reg. Reason is on x64, if i understand what you propose correctly, the following will happen

when reg - REG_R15 (value = 15)

reg = NEXT_REGISTER(reg) will result in reg - Reg_XMM0(value = 32)

The next time we do NEXT_REGISTER(reg), this will then result in us looking up regIndices[32] which will then point to REG_XMM17

Would something like the following make sense??

// REGISTER_LOOKUP : a lookup table to match register indice to active registers. #ifdef TARGET_AMD64 #define REGISTER_LOOKUP(regIndex) regIndices[(unsigned)regIndex] #else #define REGISTER_LOOKUP(regIndex) regIndex #endif

regNumber regIndex = REG_FIRST; for (regNumber reg = REGISTER_LOOKUP(regIndex); reg < AVAILABLE_REG_COUNT; regIndex = REG_NEXT(regIndex), reg = REGISTER_LOOKUP(regIndex)) { ................... }

I imagine for non x64, this will get optimized to

regNumber regIndex = REG_FIRST; for (regNumber reg = regIndex ; reg < AVAILABLE_REG_COUNT; regIndex = REG_NEXT(regIndex), reg = regIndex) { ................... }

and then

for (regNumber reg = REG_FIRST; reg < AVAILABLE_REG_COUNT; reg= REG_NEXT(reg)) { ................... }

ah yes, that's right. May be you can do something like this then?

#ifdef TARGET_XARCH #define NEXT_REGISTER(reg, index) index++, reg = regIndices[index] #else #define NEXT_REGISTER(reg, index) REG_NEXT(reg) #endif int index = 0; for (regNumber reg = REG_FIRST; reg < AVAILABLE_REG_COUNT; reg = NEXT_REGISTER(reg, index)) { ... }

If I do that, reg = NEXT_REGISTER(reg, index) will resolve to the following for `x64)

reg = index++, reg = regIndices[index]

Which is why I did the following

Definition and usage

@kunalspathak I have made the requested changes

src/coreclr/jit/targetamd64.h

This reverts commit 963c341.

src/coreclr/jit/lsra.cpp

kunalspathak · 2025-04-29T23:20:13Z

/azp run runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

azure-pipelines · 2025-04-29T23:20:32Z

Azure Pipelines successfully started running 2 pipeline(s).

kunalspathak · 2025-04-29T23:20:57Z

Just to note down latest TP numbers:

kunalspathak · 2025-04-29T23:22:00Z

@DeepakRajendrakumaran - I know you have done lot of work to improve TP. I am wondering did you perform analysis of the remaining regressions and can share where the cost is coming from?

kunalspathak · 2025-04-29T23:26:42Z

/azp run runtime-coreclr libraries-jitstress2-jitstressregs

azure-pipelines · 2025-04-29T23:26:51Z

Azure Pipelines successfully started running 1 pipeline(s).

DeepakRajendrakumaran · 2025-04-30T00:30:49Z

@DeepakRajendrakumaran - I know you have done lot of work to improve TP. I am wondering did you perform analysis of the remaining regressions and can share where the cost is coming from?

I ran this for libraries_tests.run as a sample

The tpdiff for libraries_tests.run is as follows locally

Overall (+2.12%)

Collection	PDIFF
libraries_tests.run.windows.x64.Release.mch	+2.12%

MinOpts (+4.46%)

Collection	PDIFF
libraries_tests.run.windows.x64.Release.mch	+4.46%

FullOpts (+1.58%)

Collection	PDIFF
libraries_tests.run.windows.x64.Release.mch	+1.58%

I did try some changes with separating out registers to free in allocateRegisters() and allocateRegistersMinimal(). Handling delay registers made it a little tricky. It could be something someone could circle back to later.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 27, 2025

DeepakRajendrakumaran force-pushed the allocateRegMin branch 2 times, most recently from 657a419 to 50153a7 Compare April 1, 2025 17:44

DeepakRajendrakumaran force-pushed the allocateRegMin branch 5 times, most recently from 5e5548c to 85c9ef8 Compare April 15, 2025 21:57

build-analysis bot mentioned this pull request Apr 16, 2025

[linux-x64] [mono-aot] Test Runtime_101731.TestConvertToInt64NativeSingle(3.4028235E+38) returns exit code 22 #112557

Open

DeepakRajendrakumaran force-pushed the allocateRegMin branch 4 times, most recently from 3a45505 to 963c341 Compare April 17, 2025 16:32

DeepakRajendrakumaran added 4 commits April 17, 2025 09:42

Add remaining 8 eGPR

fd5fb2f

Skip unused regs.

3bb0a4e

Revert "Skip unused regs."

5363283

This reverts commit cc6d454.

Skip unused regs

963c341

DeepakRajendrakumaran changed the title ~~Draft : Reduce TP regression in allocateRegistersMinimal()~~ Adding additional 8 eGPR. Apr 21, 2025

DeepakRajendrakumaran marked this pull request as ready for review April 21, 2025 14:47

Copilot AI review requested due to automatic review settings April 21, 2025 14:47

Copilot AI reviewed Apr 21, 2025

View reviewed changes

BruceForstall reviewed Apr 21, 2025

View reviewed changes

Addressing review comments.

cb20541

BruceForstall approved these changes Apr 21, 2025

View reviewed changes

BruceForstall requested a review from kunalspathak April 21, 2025 22:56

kunalspathak reviewed Apr 21, 2025

View reviewed changes

DeepakRajendrakumaran added 4 commits April 23, 2025 13:33

Revert "Skip unused regs"

3c38b92

This reverts commit 963c341.

Addressing Kunal's review comments.

4c6e85a

Fix formatting

93bc1dd

Merge remote-tracking branch 'origin/main' into origin/allocateRegMin

cca23f8

build-analysis bot mentioned this pull request Apr 25, 2025

LibraryImportGenerator.Unit.Tests crashing on linux-x64 mono interpreter #100800

Open

Clean up macro

247a89c

BruceForstall added the apx Related to the Intel Advanced Performance Extensions (APX) label Apr 28, 2025

DeepakRajendrakumaran added 2 commits April 28, 2025 12:12

Merge remote-tracking branch 'origin/main' into origin/allocateRegMin

bee91d4

Merge remote-tracking branch 'origin' into origin/allocateRegMin

1be98c6

kunalspathak reviewed Apr 28, 2025

View reviewed changes

src/coreclr/jit/lsra.cpp Outdated Show resolved Hide resolved

kunalspathak reviewed Apr 28, 2025

View reviewed changes

src/coreclr/jit/lsra.cpp Outdated Show resolved Hide resolved

Reverting change for arm and fixing bound

163478e

kunalspathak reviewed Apr 29, 2025

View reviewed changes

src/coreclr/jit/lsra.cpp Outdated Show resolved Hide resolved

DeepakRajendrakumaran added 2 commits April 29, 2025 09:39

Fix type and addressing review comments.

3f07022

Fix typo

374f85b

build-analysis bot mentioned this pull request Apr 29, 2025

System.Security.Cryptography.X509Certificates.Tests.PfxTests.ReadMLKem512PrivateKey_NotSupported failing with CryptographicException #115156

Open

DeepakRajendrakumaran commented Apr 29, 2025

View reviewed changes

src/coreclr/jit/lsra.cpp Show resolved Hide resolved

		// TODO-apx: the definition here is incorrect, we will need to revisit this after we extend the register definition.
		// for now, we can simply use REX2 as REX.

Adding additional 8 eGPR. #113988

Are you sure you want to change the base?

Adding additional 8 eGPR. #113988

Conversation

DeepakRajendrakumaran commented Mar 27, 2025 • edited Loading

Overview

TPDIFF impact

windows x64

Alternate test numbers from running superpmi directly and using the 'Total Time':

Copilot AI left a comment

Choose a reason for hiding this comment

DeepakRajendrakumaran commented Apr 21, 2025

BruceForstall commented Apr 21, 2025

kunalspathak commented Apr 21, 2025

DeepakRajendrakumaran commented Apr 21, 2025 • edited Loading

DeepakRajendrakumaran commented Apr 21, 2025

BruceForstall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepakRajendrakumaran Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepakRajendrakumaran Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepakRajendrakumaran Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

kunalspathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Apr 29, 2025

azure-pipelines bot commented Apr 29, 2025

kunalspathak commented Apr 29, 2025

kunalspathak commented Apr 29, 2025

kunalspathak commented Apr 29, 2025

azure-pipelines bot commented Apr 29, 2025

DeepakRajendrakumaran commented Apr 30, 2025 • edited Loading

DeepakRajendrakumaran commented Mar 27, 2025 •

edited

Loading

DeepakRajendrakumaran commented Apr 21, 2025 •

edited

Loading

DeepakRajendrakumaran Apr 21, 2025 •

edited

Loading

DeepakRajendrakumaran Apr 21, 2025 •

edited

Loading

DeepakRajendrakumaran Apr 21, 2025 •

edited

Loading

DeepakRajendrakumaran commented Apr 30, 2025 •

edited

Loading