Various fixes for consecutive registers found with jitstressregs #84824

kunalspathak · 2023-04-14T08:08:09Z

The register that was assigned previously to a copyReg scenario where not recorded as live at the location because of which we were reassigning it to a different variable, overwriting its value before consuming it for copy. This relates to my findings in [JitStress] VectorTableLookupExtension fails in jitstress1 #84696 (comment).
When we generate code for upper vector restore, we were generating it at the use where FIELD_LIST is populated. But, between that and the intrinsic node, where it is consumed, there can be any number of nodes (like GT_CALL) where the restored upper half vectors can be again wiped off. Make sure to insert the restores just before the use of the FIELD_LIST node. This relates to my findings in [JitStress] VectorTableLookupExtension fails in jitstress1 #84696 (comment).
There are numerous scenarios in which constraining the number of registers doesn't work with the consecutive registers because consecutive registers, itself constraints the registers that can be assigned. As such, I have added isLiveAtConsecutiveRegistersLoc and we will skip constraining the registers for such refpositions. However, to stress test the busy consecutive registers and free/busy code path, I have added a constraint on free registers that the algorithm sees under stress mode.

ghost · 2023-04-14T08:08:22Z

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

Issue Details

null

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

kunalspathak · 2023-04-14T08:08:36Z

/azp run runtime-coreclr jitstress

kunalspathak · 2023-04-14T08:08:44Z

/azp run runtime-coreclr jitstressregs

azure-pipelines · 2023-04-14T08:08:48Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2023-04-14T08:08:53Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2023-04-14T08:08:56Z

/azp run runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2023-04-14T08:09:07Z

Azure Pipelines successfully started running 1 pipeline(s).

This method will track if the defs/uses are live at the same location as where the consecutive registers were allocated. If yes, it will skip the constraint imposition on it during JitStressRegs

When we have copyReg that was just restored or previously assigned to a different register, also track it as live at the location so it doesn't get allocated again for different refposition at the same location.

kunalspathak · 2023-04-15T06:58:40Z

/azp run runtime-coreclr jitstress

kunalspathak · 2023-04-15T06:58:50Z

/azp run runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2023-04-15T06:58:50Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2023-04-15T06:59:00Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2023-04-15T06:59:02Z

/azp run runtime-coreclr jitstressregs

azure-pipelines · 2023-04-15T06:59:12Z

Azure Pipelines successfully started running 1 pipeline(s).

…isters Under JitStressRegs, there are multiple ways in which consecutive registers demand cannot be met. So skip restricting the registers for `registerAssignment` of a refPosition (which are allowable candidates that can be assigned to the given refposition). Instead limit the free registers to alternate under stress mode, so we can verify the code if it can handle situation where it needs to pick from a mix of free/busy registers.

kunalspathak · 2023-04-17T21:17:52Z

/azp run runtime-coreclr jitstress
/azp run runtime-coreclr jitstress2-jitstressregs
/azp run runtime-coreclr jitstress2-jitstressregs

azure-pipelines · 2023-04-17T21:17:58Z

No pipelines are associated with this pull request.

kunalspathak · 2023-04-17T21:18:47Z

/azp run runtime-coreclr jitstress

kunalspathak · 2023-04-17T21:18:53Z

/azp run runtime-coreclr jitstress2-jitstressregs

kunalspathak · 2023-04-17T21:19:03Z

/azp run runtime-coreclr jitstressregs

azure-pipelines · 2023-04-17T21:19:04Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2023-04-17T21:19:06Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2023-04-17T21:19:14Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2023-04-18T03:23:21Z

All the CI legs are passing. There is failures on x64 for which I have opened #84961. There are failures in JIT/IL_Conformance/Old/Conformance_Base/ for arm64 which are unlikely to be affected by this change. I have started a run for jitstress on main just to confirm it is not related.

kunalspathak · 2023-04-18T05:29:31Z

I have started a run for jitstress on main just to confirm it is not related.

They are on main as well. Opened #84966.

kunalspathak · 2023-04-18T05:30:21Z

@dotnet/jit-contrib @BruceForstall

src/coreclr/jit/lsra.h

src/coreclr/jit/lsraarm64.cpp

kunalspathak · 2023-04-19T14:33:13Z

ping @BruceForstall

kunalspathak · 2023-04-19T19:05:42Z

Adding the links for green runs :
jitstress2-jitstressregs , jitstressregs and jitstress

BruceForstall · 2023-04-19T20:16:20Z

src/coreclr/jit/lsra.h

@@ -1784,6 +1784,12 @@ class LinearScan : public LinearScanInterface
    void clearSpillCost(regNumber reg, var_types regType);
    void updateSpillCost(regNumber reg, Interval* interval);

+    FORCEINLINE void updateRegsFreeBusyState(RefPosition& refPosition,


Is it really necessary to use FORCEINLINE?

This is hot functionality used whenever we assign copyRegs with or without consecutive registers. I didn't want to impact the TP for just extracting out this functionality in a method.

src/coreclr/jit/lsraarm64.cpp

src/coreclr/jit/lsrabuild.cpp

src/coreclr/jit/lsra.cpp

restore the upper vector at the use of GT_FIELD_LIST

1462dc7

ghost assigned kunalspathak Apr 14, 2023

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

build-analysis bot mentioned this pull request Apr 14, 2023

Tracking issue for CI build timeouts #76454

Closed

kunalspathak added 2 commits April 14, 2023 10:23

Introduce isLiveAtConsecutiveRegistersLoc and fix dotnet#84747

0d740b2

This method will track if the defs/uses are live at the same location as where the consecutive registers were allocated. If yes, it will skip the constraint imposition on it during JitStressRegs

Handle tracking of previously assigned register for copyReg

89d39b8

When we have copyReg that was just restored or previously assigned to a different register, also track it as live at the location so it doesn't get allocated again for different refposition at the same location.

kunalspathak added 4 commits April 16, 2023 12:31

fix the release build errors

27ddde6

Mark consecutive refpositions registers as busy

2cb1937

Merge remote-tracking branch 'origin/main' into consecutive-jitstress

adacdcf

Update the comments

092a72c

runfoapp bot mentioned this pull request Apr 17, 2023

Test failure: System.Security.Cryptography.X509Certificates.Tests.CertificateCreation.CertificateRequestChainTests/CreateChain_Hybrid #25979

Closed

kunalspathak added 3 commits April 17, 2023 10:57

Introduce updateRegsFreeBusyState() for common trackign

f9cf985

Update comment

d2960c6

kunalspathak marked this pull request as ready for review April 18, 2023 05:29

kunalspathak requested a review from BruceForstall April 18, 2023 05:30

kunalspathak changed the title ~~restore the upper vector at the use of GT_FIELD_LIST~~ Various fixes for consecutive registers found with jitstressregs Apr 18, 2023

kunalspathak commented Apr 18, 2023

View reviewed changes

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

kunalspathak commented Apr 18, 2023

View reviewed changes

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

misc. changes

ab5a9c9

BruceForstall approved these changes Apr 19, 2023

View reviewed changes

review feedback

3e1ec41

kunalspathak merged commit 42acf9e into dotnet:main Apr 20, 2023

kunalspathak deleted the consecutive-jitstress branch April 20, 2023 00:06

This was referenced Apr 20, 2023

[JitStress] VectorTableLookupExtension fails in jitstress1 #84696

Closed

arm64 JitStressRegs8 VectorTableLookup test failures #84746

Closed

kunalspathak mentioned this pull request Apr 30, 2023

Do not set pre-decided consecutive registers as busy (again) #85566

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes for consecutive registers found with jitstressregs #84824

Various fixes for consecutive registers found with jitstressregs #84824

kunalspathak commented Apr 14, 2023 •

edited

Loading

ghost commented Apr 14, 2023

kunalspathak commented Apr 14, 2023

kunalspathak commented Apr 14, 2023

azure-pipelines bot commented Apr 14, 2023

azure-pipelines bot commented Apr 14, 2023

kunalspathak commented Apr 14, 2023

azure-pipelines bot commented Apr 14, 2023

kunalspathak commented Apr 15, 2023

kunalspathak commented Apr 15, 2023

azure-pipelines bot commented Apr 15, 2023

azure-pipelines bot commented Apr 15, 2023

kunalspathak commented Apr 15, 2023

azure-pipelines bot commented Apr 15, 2023

kunalspathak commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

kunalspathak commented Apr 17, 2023

kunalspathak commented Apr 17, 2023

kunalspathak commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

kunalspathak commented Apr 18, 2023

kunalspathak commented Apr 18, 2023

kunalspathak commented Apr 18, 2023

kunalspathak commented Apr 19, 2023

kunalspathak commented Apr 19, 2023

BruceForstall Apr 19, 2023

kunalspathak Apr 19, 2023

Various fixes for consecutive registers found with jitstressregs #84824

Various fixes for consecutive registers found with jitstressregs #84824

Conversation

kunalspathak commented Apr 14, 2023 • edited Loading

ghost commented Apr 14, 2023

kunalspathak commented Apr 14, 2023

kunalspathak commented Apr 14, 2023

azure-pipelines bot commented Apr 14, 2023

azure-pipelines bot commented Apr 14, 2023

kunalspathak commented Apr 14, 2023

azure-pipelines bot commented Apr 14, 2023

kunalspathak commented Apr 15, 2023

kunalspathak commented Apr 15, 2023

azure-pipelines bot commented Apr 15, 2023

azure-pipelines bot commented Apr 15, 2023

kunalspathak commented Apr 15, 2023

azure-pipelines bot commented Apr 15, 2023

kunalspathak commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

kunalspathak commented Apr 17, 2023

kunalspathak commented Apr 17, 2023

kunalspathak commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

azure-pipelines bot commented Apr 17, 2023

kunalspathak commented Apr 18, 2023

kunalspathak commented Apr 18, 2023

kunalspathak commented Apr 18, 2023

kunalspathak commented Apr 19, 2023

kunalspathak commented Apr 19, 2023

BruceForstall Apr 19, 2023

Choose a reason for hiding this comment

kunalspathak Apr 19, 2023

Choose a reason for hiding this comment

kunalspathak commented Apr 14, 2023 •

edited

Loading