Couple optimization to MultiRegStoreLoc - #64857

Merged
echesakov merged 2 commits into
dotnet:mainfrom
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged

Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom
echesakov:StoreLoc-MultiReg-PreferSrcReg

Conversation

@echesakov

@echesakovechesakov commented Feb 5, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.

Here is a description of the change:

  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    The second issue can be demonstrated by examples.

Consider the following program:

usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}

The code diffs for Main (on win-arm64):

diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])

Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.

Now, if you consider another program:

usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}

the diffs are opposite in some sense

diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])

val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.

@echesakovechesakov added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 5, 2022
@echesakovechesakov reopened this Feb 6, 2022
@echesakov

echesakov commented Feb 6, 2022

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-coreclr outerloop

@ghostghost assigned echesakovFeb 6, 2022
@azure-pipelines

azure-pipelinesBot commented Feb 6, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ghost

ghost commented Feb 6, 2022

Copy link
Copy Markdown

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

Issue Details

null

Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:

area-CodeGen-coreclr

Milestone:-

@echesakovechesakov reopened this Feb 7, 2022
@echesakov

echesakov commented Feb 7, 2022

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022
Closed
Merged
@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed the StoreLoc-MultiReg-PreferSrcReg branch from 79d3bb6 to d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov reopened this Feb 14, 2022
@dotnetdotnet deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov changed the title For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov marked this pull request as ready for review February 15, 2022 01:31
@echesakov

echesakov commented Feb 15, 2022

Copy link
Copy Markdown
ContributorAuthor

@dotnet/jit-contrib PTAL

@dotnetdotnet deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak

kunalspathak commented Feb 15, 2022

Copy link
Copy Markdown
Contributor

/azp run runtime-coreclr jitstressregs

@azure-pipelines

azure-pipelinesBot commented Feb 15, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak
kunalspathak approved these changes Feb 15, 2022
View reviewed changes

@kunalspathakkunalspathak left a comment

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.

LGTM as long as jitstressregs is clean.

@echesakov

echesakov commented Feb 15, 2022

Copy link
Copy Markdown
ContributorAuthor

LGTM as long as jitstressregs is clean.

Taking a look at the linux-arm failures.

@echesakovechesakov mentioned this pull request Feb 15, 2022
Closed
@echesakov

echesakov commented Feb 15, 2022

Copy link
Copy Markdown
ContributorAuthor

The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.
I could try to re-trigger them.

@echesakov

echesakov commented Feb 15, 2022

Copy link
Copy Markdown
ContributorAuthor

macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)

@echesakov
echesakov merged commit 19fc306 into dotnet:mainFeb 15, 2022
@echesakov
echesakov deleted the StoreLoc-MultiReg-PreferSrcReg branch February 15, 2022 23:33
@EgorBo

EgorBo commented Feb 17, 2022

Copy link
Copy Markdown
Member

Improvements on arm64: dotnet/perf-autofiling-issues#3565

@BruceForstallBruceForstall mentioned this pull request Feb 25, 2022
Closed
BruceForstall added a commit to BruceForstall/runtime that referenced this pull request Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall mentioned this pull request Feb 26, 2022
Merged
BruceForstall added a commit that referenced this pull request Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT mentioned this pull request Mar 3, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Couple optimization to MultiRegStoreLoc - #64857
Merged
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged
Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Conversation
@echesakov
@echesakovechesakov
commented
Feb 5, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.
Here is a description of the change:
  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    
The second issue can be demonstrated by examples.
Consider the following program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}
The code diffs for Main (on win-arm64):
diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.
Now, if you consider another program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}
the diffs are opposite in some sense
diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.
@echesakovechesakov
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Feb 5, 2022
@echesakovechesakov
reopened this
Feb 6, 2022
@echesakov
echesakov
commented
Feb 6, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@ghostghost
assigned echesakovFeb 6, 2022
@azure-pipelines
azure-pipelinesBot
commented
Feb 6, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ghost
ghost
commented
Feb 6, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:
area-CodeGen-coreclr
Milestone:-
@echesakovechesakov
reopened this
Feb 7, 2022
@echesakov
echesakov
commented
Feb 7, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022

Closed

 Merged

@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed
the
StoreLoc-MultiReg-PreferSrcReg
branch
from
79d3bb6 to
d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov
reopened this
Feb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov
changed the title
For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov
marked this pull request as ready for review
February 15, 2022 01:31
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib PTAL
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak
kunalspathak
commented
Feb 15, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-coreclr jitstressregs
@azure-pipelines
azure-pipelinesBot
commented
Feb 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
kunalspathak
kunalspathak
approved these changes
Feb 15, 2022
View reviewed changes
@kunalspathakkunalspathak
left a comment


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.

LGTM as long as jitstressregs is clean.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM as long as jitstressregs is clean.
Taking a look at the linux-arm failures.
@echesakovechesakov
mentioned this pull request
Feb 15, 2022

Closed

@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.

I could try to re-trigger them.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)
@echesakov
echesakov
merged commit 19fc306
into
dotnet:mainFeb 15, 2022
@echesakov
echesakov
deleted the

StoreLoc-MultiReg-PreferSrcReg

branch
February 15, 2022 23:33
@EgorBo
EgorBo
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
Improvements on arm64: dotnet/perf-autofiling-issues#3565
@BruceForstallBruceForstall
mentioned this pull request
Feb 25, 2022

Closed

BruceForstall added a commit
to BruceForstall/runtime
that referenced
this pull request
Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall
mentioned this pull request
Feb 26, 2022
 Merged

BruceForstall added a commit
that referenced
this pull request
Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Mar 3, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Couple optimization to MultiRegStoreLoc - #64857
Merged
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged
Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Conversation
@echesakov
@echesakovechesakov
commented
Feb 5, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.
Here is a description of the change:
  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    
The second issue can be demonstrated by examples.
Consider the following program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}
The code diffs for Main (on win-arm64):
diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.
Now, if you consider another program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}
the diffs are opposite in some sense
diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.
@echesakovechesakov
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Feb 5, 2022
@echesakovechesakov
reopened this
Feb 6, 2022
@echesakov
echesakov
commented
Feb 6, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@ghostghost
assigned echesakovFeb 6, 2022
@azure-pipelines
azure-pipelinesBot
commented
Feb 6, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ghost
ghost
commented
Feb 6, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:
area-CodeGen-coreclr
Milestone:-
@echesakovechesakov
reopened this
Feb 7, 2022
@echesakov
echesakov
commented
Feb 7, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022

Closed

 Merged

@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed
the
StoreLoc-MultiReg-PreferSrcReg
branch
from
79d3bb6 to
d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov
reopened this
Feb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov
changed the title
For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov
marked this pull request as ready for review
February 15, 2022 01:31
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib PTAL
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak
kunalspathak
commented
Feb 15, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-coreclr jitstressregs
@azure-pipelines
azure-pipelinesBot
commented
Feb 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
kunalspathak
kunalspathak
approved these changes
Feb 15, 2022
View reviewed changes
@kunalspathakkunalspathak
left a comment


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.

LGTM as long as jitstressregs is clean.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM as long as jitstressregs is clean.
Taking a look at the linux-arm failures.
@echesakovechesakov
mentioned this pull request
Feb 15, 2022

Closed

@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.

I could try to re-trigger them.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)
@echesakov
echesakov
merged commit 19fc306
into
dotnet:mainFeb 15, 2022
@echesakov
echesakov
deleted the

StoreLoc-MultiReg-PreferSrcReg

branch
February 15, 2022 23:33
@EgorBo
EgorBo
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
Improvements on arm64: dotnet/perf-autofiling-issues#3565
@BruceForstallBruceForstall
mentioned this pull request
Feb 25, 2022

Closed

BruceForstall added a commit
to BruceForstall/runtime
that referenced
this pull request
Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall
mentioned this pull request
Feb 26, 2022
 Merged

BruceForstall added a commit
that referenced
this pull request
Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Mar 3, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Couple optimization to MultiRegStoreLoc - #64857
Merged
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged
Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Conversation
@echesakov
@echesakovechesakov
commented
Feb 5, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.
Here is a description of the change:
  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    
The second issue can be demonstrated by examples.
Consider the following program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}
The code diffs for Main (on win-arm64):
diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.
Now, if you consider another program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}
the diffs are opposite in some sense
diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.
@echesakovechesakov
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Feb 5, 2022
@echesakovechesakov
reopened this
Feb 6, 2022
@echesakov
echesakov
commented
Feb 6, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@ghostghost
assigned echesakovFeb 6, 2022
@azure-pipelines
azure-pipelinesBot
commented
Feb 6, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ghost
ghost
commented
Feb 6, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:
area-CodeGen-coreclr
Milestone:-
@echesakovechesakov
reopened this
Feb 7, 2022
@echesakov
echesakov
commented
Feb 7, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022

Closed

 Merged

@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed
the
StoreLoc-MultiReg-PreferSrcReg
branch
from
79d3bb6 to
d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov
reopened this
Feb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov
changed the title
For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov
marked this pull request as ready for review
February 15, 2022 01:31
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib PTAL
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak
kunalspathak
commented
Feb 15, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-coreclr jitstressregs
@azure-pipelines
azure-pipelinesBot
commented
Feb 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
kunalspathak
kunalspathak
approved these changes
Feb 15, 2022
View reviewed changes
@kunalspathakkunalspathak
left a comment


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.

LGTM as long as jitstressregs is clean.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM as long as jitstressregs is clean.
Taking a look at the linux-arm failures.
@echesakovechesakov
mentioned this pull request
Feb 15, 2022

Closed

@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.

I could try to re-trigger them.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)
@echesakov
echesakov
merged commit 19fc306
into
dotnet:mainFeb 15, 2022
@echesakov
echesakov
deleted the

StoreLoc-MultiReg-PreferSrcReg

branch
February 15, 2022 23:33
@EgorBo
EgorBo
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
Improvements on arm64: dotnet/perf-autofiling-issues#3565
@BruceForstallBruceForstall
mentioned this pull request
Feb 25, 2022

Closed

BruceForstall added a commit
to BruceForstall/runtime
that referenced
this pull request
Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall
mentioned this pull request
Feb 26, 2022
 Merged

BruceForstall added a commit
that referenced
this pull request
Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Mar 3, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Couple optimization to MultiRegStoreLoc - #64857
Merged
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged
Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Conversation
@echesakov
@echesakovechesakov
commented
Feb 5, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.
Here is a description of the change:
  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    
The second issue can be demonstrated by examples.
Consider the following program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}
The code diffs for Main (on win-arm64):
diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.
Now, if you consider another program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}
the diffs are opposite in some sense
diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.
@echesakovechesakov
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Feb 5, 2022
@echesakovechesakov
reopened this
Feb 6, 2022
@echesakov
echesakov
commented
Feb 6, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@ghostghost
assigned echesakovFeb 6, 2022
@azure-pipelines
azure-pipelinesBot
commented
Feb 6, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ghost
ghost
commented
Feb 6, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:
area-CodeGen-coreclr
Milestone:-
@echesakovechesakov
reopened this
Feb 7, 2022
@echesakov
echesakov
commented
Feb 7, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022

Closed

 Merged

@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed
the
StoreLoc-MultiReg-PreferSrcReg
branch
from
79d3bb6 to
d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov
reopened this
Feb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov
changed the title
For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov
marked this pull request as ready for review
February 15, 2022 01:31
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib PTAL
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak
kunalspathak
commented
Feb 15, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-coreclr jitstressregs
@azure-pipelines
azure-pipelinesBot
commented
Feb 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
kunalspathak
kunalspathak
approved these changes
Feb 15, 2022
View reviewed changes
@kunalspathakkunalspathak
left a comment


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.

LGTM as long as jitstressregs is clean.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM as long as jitstressregs is clean.
Taking a look at the linux-arm failures.
@echesakovechesakov
mentioned this pull request
Feb 15, 2022

Closed

@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.

I could try to re-trigger them.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)
@echesakov
echesakov
merged commit 19fc306
into
dotnet:mainFeb 15, 2022
@echesakov
echesakov
deleted the

StoreLoc-MultiReg-PreferSrcReg

branch
February 15, 2022 23:33
@EgorBo
EgorBo
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
Improvements on arm64: dotnet/perf-autofiling-issues#3565
@BruceForstallBruceForstall
mentioned this pull request
Feb 25, 2022

Closed

BruceForstall added a commit
to BruceForstall/runtime
that referenced
this pull request
Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall
mentioned this pull request
Feb 26, 2022
 Merged

BruceForstall added a commit
that referenced
this pull request
Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Mar 3, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Couple optimization to MultiRegStoreLoc - #64857
Merged
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged
Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Conversation
@echesakov
@echesakovechesakov
commented
Feb 5, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.
Here is a description of the change:
  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    
The second issue can be demonstrated by examples.
Consider the following program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}
The code diffs for Main (on win-arm64):
diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.
Now, if you consider another program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}
the diffs are opposite in some sense
diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.
@echesakovechesakov
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Feb 5, 2022
@echesakovechesakov
reopened this
Feb 6, 2022
@echesakov
echesakov
commented
Feb 6, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@ghostghost
assigned echesakovFeb 6, 2022
@azure-pipelines
azure-pipelinesBot
commented
Feb 6, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ghost
ghost
commented
Feb 6, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:
area-CodeGen-coreclr
Milestone:-
@echesakovechesakov
reopened this
Feb 7, 2022
@echesakov
echesakov
commented
Feb 7, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022

Closed

 Merged

@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed
the
StoreLoc-MultiReg-PreferSrcReg
branch
from
79d3bb6 to
d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov
reopened this
Feb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov
changed the title
For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov
marked this pull request as ready for review
February 15, 2022 01:31
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib PTAL
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak
kunalspathak
commented
Feb 15, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-coreclr jitstressregs
@azure-pipelines
azure-pipelinesBot
commented
Feb 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
kunalspathak
kunalspathak
approved these changes
Feb 15, 2022
View reviewed changes
@kunalspathakkunalspathak
left a comment


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.

LGTM as long as jitstressregs is clean.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM as long as jitstressregs is clean.
Taking a look at the linux-arm failures.
@echesakovechesakov
mentioned this pull request
Feb 15, 2022

Closed

@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.

I could try to re-trigger them.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)
@echesakov
echesakov
merged commit 19fc306
into
dotnet:mainFeb 15, 2022
@echesakov
echesakov
deleted the

StoreLoc-MultiReg-PreferSrcReg

branch
February 15, 2022 23:33
@EgorBo
EgorBo
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
Improvements on arm64: dotnet/perf-autofiling-issues#3565
@BruceForstallBruceForstall
mentioned this pull request
Feb 25, 2022

Closed

BruceForstall added a commit
to BruceForstall/runtime
that referenced
this pull request
Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall
mentioned this pull request
Feb 26, 2022
 Merged

BruceForstall added a commit
that referenced
this pull request
Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Mar 3, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Couple optimization to MultiRegStoreLoc - #64857
Merged
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged
Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Conversation
@echesakov
@echesakovechesakov
commented
Feb 5, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.
Here is a description of the change:
  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    
The second issue can be demonstrated by examples.
Consider the following program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}
The code diffs for Main (on win-arm64):
diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.
Now, if you consider another program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}
the diffs are opposite in some sense
diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.
@echesakovechesakov
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Feb 5, 2022
@echesakovechesakov
reopened this
Feb 6, 2022
@echesakov
echesakov
commented
Feb 6, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@ghostghost
assigned echesakovFeb 6, 2022
@azure-pipelines
azure-pipelinesBot
commented
Feb 6, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ghost
ghost
commented
Feb 6, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:
area-CodeGen-coreclr
Milestone:-
@echesakovechesakov
reopened this
Feb 7, 2022
@echesakov
echesakov
commented
Feb 7, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022

Closed

 Merged

@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed
the
StoreLoc-MultiReg-PreferSrcReg
branch
from
79d3bb6 to
d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov
reopened this
Feb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov
changed the title
For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov
marked this pull request as ready for review
February 15, 2022 01:31
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib PTAL
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak
kunalspathak
commented
Feb 15, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-coreclr jitstressregs
@azure-pipelines
azure-pipelinesBot
commented
Feb 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
kunalspathak
kunalspathak
approved these changes
Feb 15, 2022
View reviewed changes
@kunalspathakkunalspathak
left a comment


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.

LGTM as long as jitstressregs is clean.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM as long as jitstressregs is clean.
Taking a look at the linux-arm failures.
@echesakovechesakov
mentioned this pull request
Feb 15, 2022

Closed

@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.

I could try to re-trigger them.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)
@echesakov
echesakov
merged commit 19fc306
into
dotnet:mainFeb 15, 2022
@echesakov
echesakov
deleted the

StoreLoc-MultiReg-PreferSrcReg

branch
February 15, 2022 23:33
@EgorBo
EgorBo
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
Improvements on arm64: dotnet/perf-autofiling-issues#3565
@BruceForstallBruceForstall
mentioned this pull request
Feb 25, 2022

Closed

BruceForstall added a commit
to BruceForstall/runtime
that referenced
this pull request
Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall
mentioned this pull request
Feb 26, 2022
 Merged

BruceForstall added a commit
that referenced
this pull request
Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Mar 3, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Couple optimization to MultiRegStoreLoc - #64857
Merged
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Feb 15, 2022
Merged
Couple optimization to MultiRegStoreLoc#64857
echesakov merged 2 commits into
dotnet:mainfrom 
echesakov:StoreLoc-MultiReg-PreferSrcReg
Conversation
@echesakov
@echesakovechesakov
commented
Feb 5, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
The diffs are net positive. There are some regressions. Looked at couple of them - do not see how they can be mitigated - they are due to re-shuffling of the registers in LSRA.
Here is a description of the change:
  1. Preference the source registers to the destination of multi-register GT_STORE_LCL_VAR if the source is last-use local or not a local (i.e. a multi-reg call or hardware intrinsic). The similar strategy is already done for single reg GT_STORE_LCL_VAR.
  2. Properly check for the last use of a field of a multi-reg local in LinearScan::BuildStoreLocDef().
    
The second issue can be demonstrated by examples.
Consider the following program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();Use(val._0,val._0);}}}
The code diffs for Main (on win-arm64):
diff --git a/base.txt b/diff.txt
index d080ead..b5176ec 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,31 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T00] ( 3, 3 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T01] ( 1, 1 ) long -> x1 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 8+; Lcl frame size = 0
G_M3731_IG01: ;; offset=0000H
- A9BE7BFD stp fp, lr, [sp,#-32]!- F9000FF3 str x19, [sp,#24]+ A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 2.50-G_M3731_IG02: ;; offset=000CH+ ;; bbWeight=1 PerfScore 1.50+G_M3731_IG02: ;; offset=0008H
94000000 bl Runtime_64857.Program:Def():int64x2
- AA0103F3 mov x19, x1
AA0003E1 mov x1, x0
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 3.00-G_M3731_IG03: ;; offset=001CH- F9400FF3 ldr x19, [sp,#24]- A8C27BFD ldp fp, lr, [sp],#32+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG03: ;; offset=0014H+ A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 4.00+ ;; bbWeight=1 PerfScore 2.00-; Total bytes of code 40, prolog size 12, PerfScore 13.50, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 28, prolog size 8, PerfScore 8.80, instruction count 7, allocated bytes for code 28 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
Note that in the base case a variable that corresponds to promoted val._1 was added (unnecessarily) to currentLiveVars and, as a consequence, x19 was used to keep a dead value of the variable.
Now, if you consider another program:
usingSystem;usingSystem.Runtime.CompilerServices;namespaceRuntime_64857{classProgram{structint64x2{publiculong_0;publiculong_1;}[MethodImpl(MethodImplOptions.NoInlining)]staticint64x2Def(){returndefault(int64x2);}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidTrashRegs(){}[MethodImpl(MethodImplOptions.NoInlining)]staticvoidUse(ulongx0,ulongx1){}staticvoidMain(string[]args){varval=Def();TrashRegs();Use(val._1,val._1);}}}
the diffs are opposite in some sense
diff --git a/base.txt b/diff.txt
index c3b5ab8..181e57b 100644
--- a/base.txt+++ b/diff.txt@@ -11,34 +11,36 @@
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct (16) zero-ref do-not-enreg[SBR] multireg-ret "Return value temp for multireg return"
; V04 tmp2 [V04,T01] ( 1, 1 ) long -> x0 V01._0(offs=0x00) P-INDEP "field V01._0 (fldOffset=0x0)"
-; V05 tmp3 [V05,T00] ( 3, 3 ) long -> [fp+18H] V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"+; V05 tmp3 [V05,T00] ( 3, 3 ) long -> x19 V01._1(offs=0x08) P-INDEP "field V01._1 (fldOffset=0x8)"
;* V06 tmp4 [V06 ] ( 0, 0 ) long -> zero-ref V03._0(offs=0x00) P-DEP "field V03._0 (fldOffset=0x0)"
;* V07 tmp5 [V07 ] ( 0, 0 ) long -> zero-ref V03._1(offs=0x08) P-DEP "field V03._1 (fldOffset=0x8)"
;
-; Lcl frame size = 16+; Lcl frame size = 8
G_M3731_IG01: ;; offset=0000H
A9BE7BFD stp fp, lr, [sp,#-32]!
+ F9000FF3 str x19, [sp,#24]
910003FD mov fp, sp
- ;; bbWeight=1 PerfScore 1.50-G_M3731_IG02: ;; offset=0008H+ ;; bbWeight=1 PerfScore 2.50+G_M3731_IG02: ;; offset=000CH
94000000 bl Runtime_64857.Program:Def():int64x2
- F9000FA1 str x1, [fp,#24]+ AA0103F3 mov x19, x1
94000000 bl Runtime_64857.Program:TrashRegs()
- F9400FA0 ldr x0, [fp,#24]	// [V05 tmp3]- AA0003E1 mov x1, x0+ AA1303E0 mov x0, x19+ AA1303E1 mov x1, x19
94000000 bl Runtime_64857.Program:Use(long,long)
- ;; bbWeight=1 PerfScore 6.50-G_M3731_IG03: ;; offset=0020H+ ;; bbWeight=1 PerfScore 4.50+G_M3731_IG03: ;; offset=0024H+ F9400FF3 ldr x19, [sp,#24]
A8C27BFD ldp fp, lr, [sp],#32
D65F03C0 ret lr
- ;; bbWeight=1 PerfScore 2.00+ ;; bbWeight=1 PerfScore 4.00-; Total bytes of code 40, prolog size 8, PerfScore 14.00, instruction count 10, allocated bytes for code 40 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])+; Total bytes of code 48, prolog size 12, PerfScore 15.80, instruction count 12, allocated bytes for code 48 (MethodHash=8297f16c) for method Runtime_64857.Program:Main(System.String[])
val._1 was NOT added to currentLiveVars and instead of assigning a callee-saved register to val._1 (to be able to survive a call to TrashRegs()) the value had to be spilled on the stack.
@echesakovechesakov
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Feb 5, 2022
@echesakovechesakov
reopened this
Feb 6, 2022
@echesakov
echesakov
commented
Feb 6, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@ghostghost
assigned echesakovFeb 6, 2022
@azure-pipelines
azure-pipelinesBot
commented
Feb 6, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ghost
ghost
commented
Feb 6, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:echesakovMSFT
Assignees:echesakovMSFT
Labels:
area-CodeGen-coreclr
Milestone:-
@echesakovechesakov
reopened this
Feb 7, 2022
@echesakov
echesakov
commented
Feb 7, 2022

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop

This was referenced Feb 7, 2022

Closed

 Merged

@echesakov
For StoreLoc prefer allocating a multireg local def in the same regis…
cc874c5
…ters as src in src/coreclr/jit/lsrabuild.cpp
@echesakov
Properly check for the last use of a field of a multi-reg local in Li…
d94f2b8
…nearScan::BuildStoreLocDef() in src/coreclr/jit/lsrabuild.cpp
@echesakov
echesakovforce-pushed
the
StoreLoc-MultiReg-PreferSrcReg
branch
from
79d3bb6 to
d94f2b8CompareFebruary 11, 2022 19:07
@echesakovechesakov
reopened this
Feb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 14, 2022
@echesakovechesakov
changed the title
For MultiRegStoreLoc prefer allocating a local def to the same register as srcCouple optimization to MultiRegStoreLocFeb 15, 2022
@echesakov
echesakov
marked this pull request as ready for review
February 15, 2022 01:31
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib PTAL
@dotnetdotnet
deleted a comment from azure-pipelinesBotFeb 15, 2022
@kunalspathak
kunalspathak
commented
Feb 15, 2022

Copy link


Copy Markdown

Contributor
/azp run runtime-coreclr jitstressregs
@azure-pipelines
azure-pipelinesBot
commented
Feb 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
kunalspathak
kunalspathak
approved these changes
Feb 15, 2022
View reviewed changes
@kunalspathakkunalspathak
left a comment


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.

LGTM as long as jitstressregs is clean.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
LGTM as long as jitstressregs is clean.
Taking a look at the linux-arm failures.
@echesakovechesakov
mentioned this pull request
Feb 15, 2022

Closed

@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
The failures on linux-arm are known #65395 and macos-x64 ones look like an infrastructure hiccup.

I could try to re-trigger them.
@echesakov
echesakov
commented
Feb 15, 2022

Copy link


Copy Markdown

ContributorAuthor
macos-x64 is clean after re-run (https://dev.azure.com/dnceng/public/_build/results?buildId=1612824&view=logs&j=4621ec7e-3d9b-5fdb-df5a-cf7177b660c1)
@echesakov
echesakov
merged commit 19fc306
into
dotnet:mainFeb 15, 2022
@echesakov
echesakov
deleted the

StoreLoc-MultiReg-PreferSrcReg

branch
February 15, 2022 23:33
@EgorBo
EgorBo
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
Improvements on arm64: dotnet/perf-autofiling-issues#3565
@BruceForstallBruceForstall
mentioned this pull request
Feb 25, 2022

Closed

BruceForstall added a commit
to BruceForstall/runtime
that referenced
this pull request
Feb 26, 2022
@BruceForstall
Fix GC hole with multi-reg local var stores
f7fea2f
Change dotnet#64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixesdotnet#65476.
@BruceForstallBruceForstall
mentioned this pull request
Feb 26, 2022
 Merged

BruceForstall added a commit
that referenced
this pull request
Mar 1, 2022
@BruceForstall@kunalspathak
Fix GC hole with multi-reg local var stores (#65916)
8686d06
* Fix GC hole with multi-reg local var stores
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()`
case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case.
I made a number of cleanup changes along the way:
1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP
2. Marked some functions `const`
3. Improved some comments
4. Changed "ith" to "i'th" in comments which still doesn't read great,
but at least I'm not left trying to parse "ith" as an English word.
5. Use `OperIsScalarLocal()` more broadly
6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear
it only applies to the multi-reg case.
Fixes#65476.
* Update src/coreclr/jit/gentree.cpp
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Mar 3, 2022

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 19, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@echesakovechesakov

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@echesakov@kunalspathak@EgorBo