Update handling of limited register during consecutive registers allocation #84588

kunalspathak · 2023-04-10T23:08:23Z

Earlier, I added a limit mask to include few registers in case we are allocating during consecutive registers. However, the mask was not sufficient, and we were still running into situation where we won't have register to allocate to one of the consecutive registers or a refPosition that is live at the same location. Updated the handling of such cases:

Fix issue where we might have more than one busy register range available to allocate and in prevRegOpt heuristic, teach it to handle the free registers.
If we see that consecutive registers are not available in JitStressRegs mode, add back a register range so it can allocate for it.
Likewise, if a refposition that is alive at the same location where consecutive registers are live, most of the registers are busy and hence, just do not further limit the registers for such refpositions.
While calculating minRegCount, take into account the refpositions for consecutive registers.
With all of that, we do not need LsraLimitFPSetForConsecutive.

Fixes: #84536

…illcost/prevReg

If we find out that there are no candidates free/busy for refPositions that need consecutive registers, have at least one range of registers in the candidates such that allocation is possible.

Intially, we were just returning RBM_NONE if we don't find any freeCandidates, but instead should try if we can find out if there are any busy candidates that we should try them out.

…osition If consecutive registers are being allocated, other refpositions that are live at the same location might not have enough registers left to be assigned because all registers are busy. As such, introduce a way to track if we are assigning at the location of consecutive registers, and if yes, do not take jitstressregs limit into account.

For consecutive register, also include the register count needed for "minimum register requirement" when limiting the registers.

With other conditions in place, no need to have LsraLimitFPSetForConsecutive.

ghost · 2023-04-10T23:08:38Z

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

Issue Details

Earlier, I added a limit mask to include few registers in case we are allocating during consecutive registers. However, the mask was not sufficient, and we were still running into situation where we won't have register to allocate to one of the consecutive registers or a refPosition that is live at the same location. Updated the handling of such cases:

Fix issue where we might have more than one busy register range available to allocate and in prevRegOpt heuristic, teach it to handle the free registers.
If we see that consecutive registers are not available in JitStressRegs mode, add back a register range so it can allocate for it.
Likewise, if a refposition that is alive at the same location where consecutive registers are live, most of the registers are busy and hence, just do not further limit the registers for such refpositions.
While calculating minRegCount, take into account the refpositions for consecutive registers.
With all of that, we do not need LsraLimitFPSetForConsecutive.

Fixes: #84536

Author:	kunalspathak
Assignees:	kunalspathak
Labels:	`area-CodeGen-coreclr`
Milestone:	-

BruceForstall

Couple questions

BruceForstall · 2023-04-10T23:23:06Z

src/coreclr/jit/lsra.cpp

        candidates &= ~busyRegs;

+#ifdef DEBUG
+        inUseOrBusyRegsMask |= ~busyRegs;


Shouldn't this be:

Suggested change

inUseOrBusyRegsMask |= ~busyRegs;

inUseOrBusyRegsMask |= busyRegs;

?

Below, I am using this as registerAssignment & inUseOrBusyRegsMask.

Should there be a separate one for just inUseRegsMask to help differentiate?

It's confusing to see clearing all the busy regs here given the local name.

I have incorporated Bruce's feedback.

BruceForstall · 2023-04-10T23:23:37Z

src/coreclr/jit/lsra.cpp

            {
                candidates &= ~checkConflictBit;
+#ifdef DEBUG
+                inUseOrBusyRegsMask |= ~checkConflictBit;


Suggested change

inUseOrBusyRegsMask |= ~checkConflictBit;

inUseOrBusyRegsMask |= checkConflictBit;

?

Below, I am using this as registerAssignment & inUseOrBusyRegsMask.

But what if you | together two different conflict bits? Then all the bits will be set and registerAssignment & inUseOrBusyRegsMask will do nothing. Is that right? Don't you need to | together all the bits first and then registerAssignment & ~inUseOrBusyRegsMask? At the very least, inUseOrBusyRegsMask as a name doesn't make sense since in your usage it's (kind of) the opposite of that.

That's true.

src/coreclr/jit/lsra.cpp

…regs

kunalspathak · 2023-04-11T04:37:39Z

Seems build failures were fixed by #84595.

kunalspathak · 2023-04-11T17:57:42Z

superpmi errors is ~~are #84536 and~~ #84631

tannergooding · 2023-04-11T17:59:58Z

superpmi errors are #84536

Isn't this PR supposed to fix this failure in particular?

kunalspathak · 2023-04-11T18:01:12Z

superpmi errors are #84536

Isn't this PR supposed to fix this failure in particular?

Ah, meant to have this comment for the other PR. Will update my comment.

tannergooding · 2023-04-11T23:16:55Z

-- As with #84634 (comment), it looks like the SPMI asserts that are still present are from \base\.

This PR, compared to a previous PR has "half" the asserts with older PRs listing failures for both \base\ and \diff\.

kunalspathak added 8 commits April 9, 2023 10:25

Fix issue to handle candidates that don't have assignedInterval in sp…

d452147

…illcost/prevReg

Do not let limitRegs reduce the number of candidates

1708964

If we find out that there are no candidates free/busy for refPositions that need consecutive registers, have at least one range of registers in the candidates such that allocation is possible.

getConsecutiveCandidates update if freeCandidate=RBM_NONE

c31cfbd

Intially, we were just returning RBM_NONE if we don't find any freeCandidates, but instead should try if we can find out if there are any busy candidates that we should try them out.

Update minRegCount for registerAssignment

13e85ac

For consecutive register, also include the register count needed for "minimum register requirement" when limiting the registers.

Remove LsraLimitFPSetForConsecutive

f2cda1e

With other conditions in place, no need to have LsraLimitFPSetForConsecutive.

Added an assert

c0c3f4a

misc changes

01ec812

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 10, 2023

ghost assigned kunalspathak Apr 10, 2023

kunalspathak requested a review from BruceForstall April 10, 2023 23:08

sbomer mentioned this pull request Apr 11, 2023

Revert "Add SIMD to LowerCallMemcmp" #84595

Merged

BruceForstall reviewed Apr 11, 2023

View reviewed changes

kunalspathak added 2 commits April 10, 2023 21:25

jit format

e5698cc

Merge remote-tracking branch 'origin/main' into consecutive_jitstress…

fe4f675

…regs

kunalspathak closed this Apr 11, 2023

kunalspathak reopened this Apr 11, 2023

kunalspathak added 2 commits April 10, 2023 22:00

Use BitOperations::BitScanForward()

2a3d0f2

review feedback

3f5dfd7

BruceForstall approved these changes Apr 11, 2023

View reviewed changes

tannergooding approved these changes Apr 11, 2023

View reviewed changes

jakobbotsch mentioned this pull request Apr 11, 2023

fix gtSplitTree assert #84634

Merged

kunalspathak merged commit 7a57f6d into dotnet:main Apr 11, 2023

kunalspathak deleted the consecutive_jitstressregs branch April 11, 2023 23:54

kunalspathak mentioned this pull request Apr 13, 2023

Assertion failed 'refPosition->RegOptional()' #84747

Closed

ghost locked as resolved and limited conversation to collaborators May 12, 2023

	inUseOrBusyRegsMask \|= ~busyRegs;
	inUseOrBusyRegsMask \|= busyRegs;

	inUseOrBusyRegsMask \|= ~checkConflictBit;
	inUseOrBusyRegsMask \|= checkConflictBit;

Update handling of limited register during consecutive registers allocation #84588

Update handling of limited register during consecutive registers allocation #84588

Uh oh!

Conversation

kunalspathak commented Apr 10, 2023

Uh oh!

ghost commented Apr 10, 2023

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kunalspathak commented Apr 11, 2023

Uh oh!

kunalspathak commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Apr 11, 2023

Uh oh!

kunalspathak commented Apr 11, 2023

Uh oh!

tannergooding commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kunalspathak commented Apr 11, 2023 •

edited

Loading