unroll byref struct copies - #86820

Merged
markples merged 5 commits into
dotnet:mainfrom
markples:w80086
Jul 18, 2023
Merged

unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom
markples:w80086

Conversation

@markples

@markplesmarkples commented May 26, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).

This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.

Example:

staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 26, 2023
@ghostghost assigned markplesMay 26, 2023
@ghost

ghost commented May 26, 2023

Copy link
Copy Markdown

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

Issue Details

null

Author:markples
Assignees:markples
Labels:

area-CodeGen-coreclr

Milestone:-

@markplesmarkples mentioned this pull request May 26, 2023
Closed
@markplesmarkples added this to the 8.0.0 milestone May 26, 2023
@ghostghost closed this Jun 25, 2023
@ghost

ghost commented Jun 25, 2023

Copy link
Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@markplesmarkples reopened this Jul 15, 2023
@markples

markples commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Diff results - lots of interference from the negative diffs hitting all PRs right now

linux arm64

Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).

Overall (-4,320 bytes)
MinOpts (-2,748 bytes)
FullOpts (-1,572 bytes)

linux x64

Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).

Overall (+1,189 bytes)
MinOpts (-422 bytes)
FullOpts (+1,611 bytes)

windows arm64

Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).

Overall (-4,164 bytes)
MinOpts (-2,100 bytes)
FullOpts (-2,064 bytes)

windows x64

Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).

Overall (-4,641 bytes)
MinOpts (-4,438 bytes)
FullOpts (-203 bytes)

linux arm

Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).

MISSED contexts: 33,754 (3.99%)

Overall (-10,776 bytes)
MinOpts (-7,584 bytes)
FullOpts (-3,192 bytes)

windows x86

Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).

Overall (+2,116 bytes)
MinOpts (+129 bytes)
FullOpts (+1,987 bytes)

@markples

markples commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

good diff from arm64

- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples

markples commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

similar on x64

- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples

markples commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

occasional size regression

+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples marked this pull request as ready for review July 17, 2023 19:11
@markples

markples commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

@dotnet/jit-contrib
cc @clamp03@shushanhf since there are architecture-specific changes

@markples

markples commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Jul 17, 2023

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

@clamp03

clamp03 commented Jul 18, 2023

Copy link
Copy Markdown
Member

@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes

Thank you! It looks good to me. CC @t-mustafin@alpencolt

jakobbotsch
jakobbotsch reviewed Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj
Comment on lines +1 to +15
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<!-- Needed for CLRTestEnvironmentVariable -->
<RequiresProcessIsolation>true</RequiresProcessIsolation>

<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />

<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
<CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
</ItemGroup>
</Project>

@jakobbotschjakobbotschJul 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?

@markplesmarkplesJul 18, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?

@jakobbotschjakobbotschJul 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?

PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:

runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382 in ac3979a

${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}:
scenarios:
- no_tiered_compilation
${{ else }}:
scenarios:
- normal
- no_tiered_compilation

So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.

@markplesmarkplesJul 18, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.

@jakobbotsch

jakobbotsch commented Jul 18, 2023

Copy link
Copy Markdown
Member

/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay

@azure-pipelines

azure-pipelinesBot commented Jul 18, 2023

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

jakobbotsch
jakobbotsch reviewed Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp Outdated
Comment on lines +426 to +429
unsigned slots = this->GetSlotCount();
for (unsigned i = 0; i < slots; i++)
{
if (this->IsGCByRef(i))

@jakobbotschjakobbotschJul 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.

@markples

markples commented Jul 18, 2023

Copy link
Copy Markdown
ContributorAuthor

cleaner results;

linux arm64

Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).

Overall (-5,232 bytes)
MinOpts (-3,260 bytes)
FullOpts (-1,972 bytes)

linux x64

Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).

Overall (-1,142 bytes)
MinOpts (-750 bytes)
FullOpts (-392 bytes)

osx arm64

Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).

Overall (-5,732 bytes)
MinOpts (-3,276 bytes)
FullOpts (-2,456 bytes)

windows arm64

Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).

Overall (-4,416 bytes)
MinOpts (-2,580 bytes)
FullOpts (-1,836 bytes)

windows x64

Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).

MISSED contexts: 3 (0.00%)

Overall (-8,993 bytes)
MinOpts (-5,772 bytes)
FullOpts (-3,221 bytes)

linux arm

Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).

MISSED contexts: 44,200 (3.34%)

Overall (-13,518 bytes)
MinOpts (-9,530 bytes)
FullOpts (-3,988 bytes)

windows x86

Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).

No diffs found.

@markples
markples merged commit 8724933 into dotnet:mainJul 18, 2023
@markplesmarkples mentioned this pull request Jul 18, 2023
Closed
@xtqqczze

xtqqczze commented Jul 18, 2023

Copy link
Copy Markdown
Contributor
staticSpan<int>Copy1(Span<int>s)=>s;

Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G

@markples

markples commented Jul 18, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).

IDisposable
IDisposable reviewed Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
unsigned slots = GetSlotCount();
for (unsigned i = 0; i < slots; i++)

@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?

@markplesmarkplesAug 8, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.

@markplesmarkples mentioned this pull request Aug 31, 2023
Closed
@ghostghost locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes
+1 more reviewer
@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable
, '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
unroll byref struct copies - #86820
Merged
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Jul 18, 2023
Merged
unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Conversation
@markples
@markplesmarkples
commented
May 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).
This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.
Example:
staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
May 26, 2023
@ghostghost
assigned markplesMay 26, 2023
@ghost
ghost
commented
May 26, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:markples
Assignees:markples
Labels:
area-CodeGen-coreclr
Milestone:-
@markplesmarkples
mentioned this pull request
May 26, 2023

Closed

@markplesmarkples
added this to the 8.0.0 milestone
May 26, 2023
@ghostghost
closed this
Jun 25, 2023
@ghost
ghost
commented
Jun 25, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@markplesmarkples
reopened this
Jul 15, 2023
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Diff results - lots of interference from the negative diffs hitting all PRs right now
linux arm64
Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).
Overall (-4,320 bytes)

MinOpts (-2,748 bytes)

FullOpts (-1,572 bytes)
linux x64
Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).
Overall (+1,189 bytes)

MinOpts (-422 bytes)

FullOpts (+1,611 bytes)
windows arm64
Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).
Overall (-4,164 bytes)

MinOpts (-2,100 bytes)

FullOpts (-2,064 bytes)
windows x64
Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).
Overall (-4,641 bytes)

MinOpts (-4,438 bytes)

FullOpts (-203 bytes)
linux arm
Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).
MISSED contexts: 33,754 (3.99%)
Overall (-10,776 bytes)

MinOpts (-7,584 bytes)

FullOpts (-3,192 bytes)
windows x86
Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).
Overall (+2,116 bytes)

MinOpts (+129 bytes)

FullOpts (+1,987 bytes)
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
good diff from arm64
- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
similar on x64
- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
occasional size regression
+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples
marked this pull request as ready for review
July 17, 2023 19:11
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib

cc @clamp03@shushanhf since there are architecture-specific changes
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Jul 17, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@clamp03
clamp03
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes
Thank you! It looks good to me. CC @t-mustafin@alpencolt
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj

Comment on lines

+1
 to 
+15

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <!-- Needed for CLRTestEnvironmentVariable -->
 <RequiresProcessIsolation>true</RequiresProcessIsolation>

 <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
 <Optimize>True</Optimize>
 </PropertyGroup>
 <ItemGroup>
 <Compile Include="$(MSBuildProjectName).cs" />

 <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
 <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
 </ItemGroup>
</Project>
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.
I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:
runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382
in
ac3979a
${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}: 
scenarios: 
 - no_tiered_compilation
${{ else }}: 
scenarios: 
 - normal
 - no_tiered_compilation
So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.
@jakobbotsch
jakobbotsch
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay
@azure-pipelines
azure-pipelinesBot
commented
Jul 18, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
Outdated


Comment on lines

+426
 to 
+429

 unsigned slots = this->GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
 {
 if (this->IsGCByRef(i))
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
cleaner results;
linux arm64
Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).
Overall (-5,232 bytes)

MinOpts (-3,260 bytes)

FullOpts (-1,972 bytes)
linux x64
Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).
Overall (-1,142 bytes)

MinOpts (-750 bytes)

FullOpts (-392 bytes)
osx arm64
Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).
Overall (-5,732 bytes)

MinOpts (-3,276 bytes)

FullOpts (-2,456 bytes)
windows arm64
Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).
Overall (-4,416 bytes)

MinOpts (-2,580 bytes)

FullOpts (-1,836 bytes)
windows x64
Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).
MISSED contexts: 3 (0.00%)
Overall (-8,993 bytes)

MinOpts (-5,772 bytes)

FullOpts (-3,221 bytes)
linux arm
Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).
MISSED contexts: 44,200 (3.34%)
Overall (-13,518 bytes)

MinOpts (-9,530 bytes)

FullOpts (-3,988 bytes)
windows x86
Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).
No diffs found.
@markples
markples
merged commit 8724933
into
dotnet:mainJul 18, 2023
@markplesmarkples
mentioned this pull request
Jul 18, 2023
 Closed

@xtqqczze
xtqqczze
commented
Jul 18, 2023

Copy link


Copy Markdown

Contributor
staticSpan<int>Copy1(Span<int>s)=>s;
Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).
IDisposable
IDisposable
reviewed
Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
 unsigned slots = GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?
@markplesmarkplesAug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.
@markplesmarkples
mentioned this pull request
Aug 31, 2023

Closed

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


Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes

+1 more reviewer

@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable

, '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
unroll byref struct copies - #86820
Merged
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Jul 18, 2023
Merged
unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Conversation
@markples
@markplesmarkples
commented
May 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).
This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.
Example:
staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
May 26, 2023
@ghostghost
assigned markplesMay 26, 2023
@ghost
ghost
commented
May 26, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:markples
Assignees:markples
Labels:
area-CodeGen-coreclr
Milestone:-
@markplesmarkples
mentioned this pull request
May 26, 2023

Closed

@markplesmarkples
added this to the 8.0.0 milestone
May 26, 2023
@ghostghost
closed this
Jun 25, 2023
@ghost
ghost
commented
Jun 25, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@markplesmarkples
reopened this
Jul 15, 2023
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Diff results - lots of interference from the negative diffs hitting all PRs right now
linux arm64
Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).
Overall (-4,320 bytes)

MinOpts (-2,748 bytes)

FullOpts (-1,572 bytes)
linux x64
Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).
Overall (+1,189 bytes)

MinOpts (-422 bytes)

FullOpts (+1,611 bytes)
windows arm64
Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).
Overall (-4,164 bytes)

MinOpts (-2,100 bytes)

FullOpts (-2,064 bytes)
windows x64
Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).
Overall (-4,641 bytes)

MinOpts (-4,438 bytes)

FullOpts (-203 bytes)
linux arm
Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).
MISSED contexts: 33,754 (3.99%)
Overall (-10,776 bytes)

MinOpts (-7,584 bytes)

FullOpts (-3,192 bytes)
windows x86
Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).
Overall (+2,116 bytes)

MinOpts (+129 bytes)

FullOpts (+1,987 bytes)
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
good diff from arm64
- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
similar on x64
- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
occasional size regression
+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples
marked this pull request as ready for review
July 17, 2023 19:11
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib

cc @clamp03@shushanhf since there are architecture-specific changes
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Jul 17, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@clamp03
clamp03
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes
Thank you! It looks good to me. CC @t-mustafin@alpencolt
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj

Comment on lines

+1
 to 
+15

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <!-- Needed for CLRTestEnvironmentVariable -->
 <RequiresProcessIsolation>true</RequiresProcessIsolation>

 <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
 <Optimize>True</Optimize>
 </PropertyGroup>
 <ItemGroup>
 <Compile Include="$(MSBuildProjectName).cs" />

 <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
 <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
 </ItemGroup>
</Project>
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.
I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:
runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382
in
ac3979a
${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}: 
scenarios: 
 - no_tiered_compilation
${{ else }}: 
scenarios: 
 - normal
 - no_tiered_compilation
So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.
@jakobbotsch
jakobbotsch
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay
@azure-pipelines
azure-pipelinesBot
commented
Jul 18, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
Outdated


Comment on lines

+426
 to 
+429

 unsigned slots = this->GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
 {
 if (this->IsGCByRef(i))
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
cleaner results;
linux arm64
Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).
Overall (-5,232 bytes)

MinOpts (-3,260 bytes)

FullOpts (-1,972 bytes)
linux x64
Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).
Overall (-1,142 bytes)

MinOpts (-750 bytes)

FullOpts (-392 bytes)
osx arm64
Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).
Overall (-5,732 bytes)

MinOpts (-3,276 bytes)

FullOpts (-2,456 bytes)
windows arm64
Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).
Overall (-4,416 bytes)

MinOpts (-2,580 bytes)

FullOpts (-1,836 bytes)
windows x64
Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).
MISSED contexts: 3 (0.00%)
Overall (-8,993 bytes)

MinOpts (-5,772 bytes)

FullOpts (-3,221 bytes)
linux arm
Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).
MISSED contexts: 44,200 (3.34%)
Overall (-13,518 bytes)

MinOpts (-9,530 bytes)

FullOpts (-3,988 bytes)
windows x86
Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).
No diffs found.
@markples
markples
merged commit 8724933
into
dotnet:mainJul 18, 2023
@markplesmarkples
mentioned this pull request
Jul 18, 2023
 Closed

@xtqqczze
xtqqczze
commented
Jul 18, 2023

Copy link


Copy Markdown

Contributor
staticSpan<int>Copy1(Span<int>s)=>s;
Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).
IDisposable
IDisposable
reviewed
Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
 unsigned slots = GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?
@markplesmarkplesAug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.
@markplesmarkples
mentioned this pull request
Aug 31, 2023

Closed

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


Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes

+1 more reviewer

@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable

, '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
unroll byref struct copies - #86820
Merged
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Jul 18, 2023
Merged
unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Conversation
@markples
@markplesmarkples
commented
May 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).
This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.
Example:
staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
May 26, 2023
@ghostghost
assigned markplesMay 26, 2023
@ghost
ghost
commented
May 26, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:markples
Assignees:markples
Labels:
area-CodeGen-coreclr
Milestone:-
@markplesmarkples
mentioned this pull request
May 26, 2023

Closed

@markplesmarkples
added this to the 8.0.0 milestone
May 26, 2023
@ghostghost
closed this
Jun 25, 2023
@ghost
ghost
commented
Jun 25, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@markplesmarkples
reopened this
Jul 15, 2023
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Diff results - lots of interference from the negative diffs hitting all PRs right now
linux arm64
Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).
Overall (-4,320 bytes)

MinOpts (-2,748 bytes)

FullOpts (-1,572 bytes)
linux x64
Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).
Overall (+1,189 bytes)

MinOpts (-422 bytes)

FullOpts (+1,611 bytes)
windows arm64
Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).
Overall (-4,164 bytes)

MinOpts (-2,100 bytes)

FullOpts (-2,064 bytes)
windows x64
Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).
Overall (-4,641 bytes)

MinOpts (-4,438 bytes)

FullOpts (-203 bytes)
linux arm
Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).
MISSED contexts: 33,754 (3.99%)
Overall (-10,776 bytes)

MinOpts (-7,584 bytes)

FullOpts (-3,192 bytes)
windows x86
Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).
Overall (+2,116 bytes)

MinOpts (+129 bytes)

FullOpts (+1,987 bytes)
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
good diff from arm64
- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
similar on x64
- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
occasional size regression
+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples
marked this pull request as ready for review
July 17, 2023 19:11
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib

cc @clamp03@shushanhf since there are architecture-specific changes
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Jul 17, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@clamp03
clamp03
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes
Thank you! It looks good to me. CC @t-mustafin@alpencolt
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj

Comment on lines

+1
 to 
+15

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <!-- Needed for CLRTestEnvironmentVariable -->
 <RequiresProcessIsolation>true</RequiresProcessIsolation>

 <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
 <Optimize>True</Optimize>
 </PropertyGroup>
 <ItemGroup>
 <Compile Include="$(MSBuildProjectName).cs" />

 <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
 <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
 </ItemGroup>
</Project>
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.
I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:
runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382
in
ac3979a
${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}: 
scenarios: 
 - no_tiered_compilation
${{ else }}: 
scenarios: 
 - normal
 - no_tiered_compilation
So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.
@jakobbotsch
jakobbotsch
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay
@azure-pipelines
azure-pipelinesBot
commented
Jul 18, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
Outdated


Comment on lines

+426
 to 
+429

 unsigned slots = this->GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
 {
 if (this->IsGCByRef(i))
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
cleaner results;
linux arm64
Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).
Overall (-5,232 bytes)

MinOpts (-3,260 bytes)

FullOpts (-1,972 bytes)
linux x64
Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).
Overall (-1,142 bytes)

MinOpts (-750 bytes)

FullOpts (-392 bytes)
osx arm64
Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).
Overall (-5,732 bytes)

MinOpts (-3,276 bytes)

FullOpts (-2,456 bytes)
windows arm64
Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).
Overall (-4,416 bytes)

MinOpts (-2,580 bytes)

FullOpts (-1,836 bytes)
windows x64
Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).
MISSED contexts: 3 (0.00%)
Overall (-8,993 bytes)

MinOpts (-5,772 bytes)

FullOpts (-3,221 bytes)
linux arm
Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).
MISSED contexts: 44,200 (3.34%)
Overall (-13,518 bytes)

MinOpts (-9,530 bytes)

FullOpts (-3,988 bytes)
windows x86
Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).
No diffs found.
@markples
markples
merged commit 8724933
into
dotnet:mainJul 18, 2023
@markplesmarkples
mentioned this pull request
Jul 18, 2023
 Closed

@xtqqczze
xtqqczze
commented
Jul 18, 2023

Copy link


Copy Markdown

Contributor
staticSpan<int>Copy1(Span<int>s)=>s;
Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).
IDisposable
IDisposable
reviewed
Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
 unsigned slots = GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?
@markplesmarkplesAug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.
@markplesmarkples
mentioned this pull request
Aug 31, 2023

Closed

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


Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes

+1 more reviewer

@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable

, '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
unroll byref struct copies - #86820
Merged
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Jul 18, 2023
Merged
unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Conversation
@markples
@markplesmarkples
commented
May 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).
This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.
Example:
staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
May 26, 2023
@ghostghost
assigned markplesMay 26, 2023
@ghost
ghost
commented
May 26, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:markples
Assignees:markples
Labels:
area-CodeGen-coreclr
Milestone:-
@markplesmarkples
mentioned this pull request
May 26, 2023

Closed

@markplesmarkples
added this to the 8.0.0 milestone
May 26, 2023
@ghostghost
closed this
Jun 25, 2023
@ghost
ghost
commented
Jun 25, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@markplesmarkples
reopened this
Jul 15, 2023
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Diff results - lots of interference from the negative diffs hitting all PRs right now
linux arm64
Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).
Overall (-4,320 bytes)

MinOpts (-2,748 bytes)

FullOpts (-1,572 bytes)
linux x64
Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).
Overall (+1,189 bytes)

MinOpts (-422 bytes)

FullOpts (+1,611 bytes)
windows arm64
Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).
Overall (-4,164 bytes)

MinOpts (-2,100 bytes)

FullOpts (-2,064 bytes)
windows x64
Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).
Overall (-4,641 bytes)

MinOpts (-4,438 bytes)

FullOpts (-203 bytes)
linux arm
Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).
MISSED contexts: 33,754 (3.99%)
Overall (-10,776 bytes)

MinOpts (-7,584 bytes)

FullOpts (-3,192 bytes)
windows x86
Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).
Overall (+2,116 bytes)

MinOpts (+129 bytes)

FullOpts (+1,987 bytes)
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
good diff from arm64
- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
similar on x64
- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
occasional size regression
+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples
marked this pull request as ready for review
July 17, 2023 19:11
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib

cc @clamp03@shushanhf since there are architecture-specific changes
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Jul 17, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@clamp03
clamp03
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes
Thank you! It looks good to me. CC @t-mustafin@alpencolt
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj

Comment on lines

+1
 to 
+15

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <!-- Needed for CLRTestEnvironmentVariable -->
 <RequiresProcessIsolation>true</RequiresProcessIsolation>

 <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
 <Optimize>True</Optimize>
 </PropertyGroup>
 <ItemGroup>
 <Compile Include="$(MSBuildProjectName).cs" />

 <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
 <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
 </ItemGroup>
</Project>
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.
I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:
runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382
in
ac3979a
${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}: 
scenarios: 
 - no_tiered_compilation
${{ else }}: 
scenarios: 
 - normal
 - no_tiered_compilation
So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.
@jakobbotsch
jakobbotsch
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay
@azure-pipelines
azure-pipelinesBot
commented
Jul 18, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
Outdated


Comment on lines

+426
 to 
+429

 unsigned slots = this->GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
 {
 if (this->IsGCByRef(i))
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
cleaner results;
linux arm64
Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).
Overall (-5,232 bytes)

MinOpts (-3,260 bytes)

FullOpts (-1,972 bytes)
linux x64
Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).
Overall (-1,142 bytes)

MinOpts (-750 bytes)

FullOpts (-392 bytes)
osx arm64
Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).
Overall (-5,732 bytes)

MinOpts (-3,276 bytes)

FullOpts (-2,456 bytes)
windows arm64
Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).
Overall (-4,416 bytes)

MinOpts (-2,580 bytes)

FullOpts (-1,836 bytes)
windows x64
Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).
MISSED contexts: 3 (0.00%)
Overall (-8,993 bytes)

MinOpts (-5,772 bytes)

FullOpts (-3,221 bytes)
linux arm
Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).
MISSED contexts: 44,200 (3.34%)
Overall (-13,518 bytes)

MinOpts (-9,530 bytes)

FullOpts (-3,988 bytes)
windows x86
Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).
No diffs found.
@markples
markples
merged commit 8724933
into
dotnet:mainJul 18, 2023
@markplesmarkples
mentioned this pull request
Jul 18, 2023
 Closed

@xtqqczze
xtqqczze
commented
Jul 18, 2023

Copy link


Copy Markdown

Contributor
staticSpan<int>Copy1(Span<int>s)=>s;
Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).
IDisposable
IDisposable
reviewed
Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
 unsigned slots = GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?
@markplesmarkplesAug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.
@markplesmarkples
mentioned this pull request
Aug 31, 2023

Closed

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


Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes

+1 more reviewer

@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable

, '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
unroll byref struct copies - #86820
Merged
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Jul 18, 2023
Merged
unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Conversation
@markples
@markplesmarkples
commented
May 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).
This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.
Example:
staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
May 26, 2023
@ghostghost
assigned markplesMay 26, 2023
@ghost
ghost
commented
May 26, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:markples
Assignees:markples
Labels:
area-CodeGen-coreclr
Milestone:-
@markplesmarkples
mentioned this pull request
May 26, 2023

Closed

@markplesmarkples
added this to the 8.0.0 milestone
May 26, 2023
@ghostghost
closed this
Jun 25, 2023
@ghost
ghost
commented
Jun 25, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@markplesmarkples
reopened this
Jul 15, 2023
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Diff results - lots of interference from the negative diffs hitting all PRs right now
linux arm64
Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).
Overall (-4,320 bytes)

MinOpts (-2,748 bytes)

FullOpts (-1,572 bytes)
linux x64
Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).
Overall (+1,189 bytes)

MinOpts (-422 bytes)

FullOpts (+1,611 bytes)
windows arm64
Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).
Overall (-4,164 bytes)

MinOpts (-2,100 bytes)

FullOpts (-2,064 bytes)
windows x64
Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).
Overall (-4,641 bytes)

MinOpts (-4,438 bytes)

FullOpts (-203 bytes)
linux arm
Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).
MISSED contexts: 33,754 (3.99%)
Overall (-10,776 bytes)

MinOpts (-7,584 bytes)

FullOpts (-3,192 bytes)
windows x86
Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).
Overall (+2,116 bytes)

MinOpts (+129 bytes)

FullOpts (+1,987 bytes)
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
good diff from arm64
- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
similar on x64
- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
occasional size regression
+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples
marked this pull request as ready for review
July 17, 2023 19:11
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib

cc @clamp03@shushanhf since there are architecture-specific changes
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Jul 17, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@clamp03
clamp03
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes
Thank you! It looks good to me. CC @t-mustafin@alpencolt
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj

Comment on lines

+1
 to 
+15

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <!-- Needed for CLRTestEnvironmentVariable -->
 <RequiresProcessIsolation>true</RequiresProcessIsolation>

 <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
 <Optimize>True</Optimize>
 </PropertyGroup>
 <ItemGroup>
 <Compile Include="$(MSBuildProjectName).cs" />

 <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
 <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
 </ItemGroup>
</Project>
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.
I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:
runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382
in
ac3979a
${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}: 
scenarios: 
 - no_tiered_compilation
${{ else }}: 
scenarios: 
 - normal
 - no_tiered_compilation
So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.
@jakobbotsch
jakobbotsch
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay
@azure-pipelines
azure-pipelinesBot
commented
Jul 18, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
Outdated


Comment on lines

+426
 to 
+429

 unsigned slots = this->GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
 {
 if (this->IsGCByRef(i))
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
cleaner results;
linux arm64
Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).
Overall (-5,232 bytes)

MinOpts (-3,260 bytes)

FullOpts (-1,972 bytes)
linux x64
Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).
Overall (-1,142 bytes)

MinOpts (-750 bytes)

FullOpts (-392 bytes)
osx arm64
Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).
Overall (-5,732 bytes)

MinOpts (-3,276 bytes)

FullOpts (-2,456 bytes)
windows arm64
Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).
Overall (-4,416 bytes)

MinOpts (-2,580 bytes)

FullOpts (-1,836 bytes)
windows x64
Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).
MISSED contexts: 3 (0.00%)
Overall (-8,993 bytes)

MinOpts (-5,772 bytes)

FullOpts (-3,221 bytes)
linux arm
Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).
MISSED contexts: 44,200 (3.34%)
Overall (-13,518 bytes)

MinOpts (-9,530 bytes)

FullOpts (-3,988 bytes)
windows x86
Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).
No diffs found.
@markples
markples
merged commit 8724933
into
dotnet:mainJul 18, 2023
@markplesmarkples
mentioned this pull request
Jul 18, 2023
 Closed

@xtqqczze
xtqqczze
commented
Jul 18, 2023

Copy link


Copy Markdown

Contributor
staticSpan<int>Copy1(Span<int>s)=>s;
Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).
IDisposable
IDisposable
reviewed
Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
 unsigned slots = GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?
@markplesmarkplesAug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.
@markplesmarkples
mentioned this pull request
Aug 31, 2023

Closed

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


Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes

+1 more reviewer

@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable

, '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
unroll byref struct copies - #86820
Merged
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Jul 18, 2023
Merged
unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Conversation
@markples
@markplesmarkples
commented
May 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).
This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.
Example:
staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
May 26, 2023
@ghostghost
assigned markplesMay 26, 2023
@ghost
ghost
commented
May 26, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:markples
Assignees:markples
Labels:
area-CodeGen-coreclr
Milestone:-
@markplesmarkples
mentioned this pull request
May 26, 2023

Closed

@markplesmarkples
added this to the 8.0.0 milestone
May 26, 2023
@ghostghost
closed this
Jun 25, 2023
@ghost
ghost
commented
Jun 25, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@markplesmarkples
reopened this
Jul 15, 2023
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Diff results - lots of interference from the negative diffs hitting all PRs right now
linux arm64
Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).
Overall (-4,320 bytes)

MinOpts (-2,748 bytes)

FullOpts (-1,572 bytes)
linux x64
Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).
Overall (+1,189 bytes)

MinOpts (-422 bytes)

FullOpts (+1,611 bytes)
windows arm64
Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).
Overall (-4,164 bytes)

MinOpts (-2,100 bytes)

FullOpts (-2,064 bytes)
windows x64
Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).
Overall (-4,641 bytes)

MinOpts (-4,438 bytes)

FullOpts (-203 bytes)
linux arm
Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).
MISSED contexts: 33,754 (3.99%)
Overall (-10,776 bytes)

MinOpts (-7,584 bytes)

FullOpts (-3,192 bytes)
windows x86
Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).
Overall (+2,116 bytes)

MinOpts (+129 bytes)

FullOpts (+1,987 bytes)
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
good diff from arm64
- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
similar on x64
- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
occasional size regression
+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples
marked this pull request as ready for review
July 17, 2023 19:11
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib

cc @clamp03@shushanhf since there are architecture-specific changes
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Jul 17, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@clamp03
clamp03
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes
Thank you! It looks good to me. CC @t-mustafin@alpencolt
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj

Comment on lines

+1
 to 
+15

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <!-- Needed for CLRTestEnvironmentVariable -->
 <RequiresProcessIsolation>true</RequiresProcessIsolation>

 <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
 <Optimize>True</Optimize>
 </PropertyGroup>
 <ItemGroup>
 <Compile Include="$(MSBuildProjectName).cs" />

 <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
 <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
 </ItemGroup>
</Project>
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.
I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:
runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382
in
ac3979a
${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}: 
scenarios: 
 - no_tiered_compilation
${{ else }}: 
scenarios: 
 - normal
 - no_tiered_compilation
So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.
@jakobbotsch
jakobbotsch
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay
@azure-pipelines
azure-pipelinesBot
commented
Jul 18, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
Outdated


Comment on lines

+426
 to 
+429

 unsigned slots = this->GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
 {
 if (this->IsGCByRef(i))
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
cleaner results;
linux arm64
Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).
Overall (-5,232 bytes)

MinOpts (-3,260 bytes)

FullOpts (-1,972 bytes)
linux x64
Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).
Overall (-1,142 bytes)

MinOpts (-750 bytes)

FullOpts (-392 bytes)
osx arm64
Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).
Overall (-5,732 bytes)

MinOpts (-3,276 bytes)

FullOpts (-2,456 bytes)
windows arm64
Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).
Overall (-4,416 bytes)

MinOpts (-2,580 bytes)

FullOpts (-1,836 bytes)
windows x64
Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).
MISSED contexts: 3 (0.00%)
Overall (-8,993 bytes)

MinOpts (-5,772 bytes)

FullOpts (-3,221 bytes)
linux arm
Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).
MISSED contexts: 44,200 (3.34%)
Overall (-13,518 bytes)

MinOpts (-9,530 bytes)

FullOpts (-3,988 bytes)
windows x86
Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).
No diffs found.
@markples
markples
merged commit 8724933
into
dotnet:mainJul 18, 2023
@markplesmarkples
mentioned this pull request
Jul 18, 2023
 Closed

@xtqqczze
xtqqczze
commented
Jul 18, 2023

Copy link


Copy Markdown

Contributor
staticSpan<int>Copy1(Span<int>s)=>s;
Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).
IDisposable
IDisposable
reviewed
Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
 unsigned slots = GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?
@markplesmarkplesAug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.
@markplesmarkples
mentioned this pull request
Aug 31, 2023

Closed

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


Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes

+1 more reviewer

@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable

, '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
unroll byref struct copies - #86820
Merged
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Jul 18, 2023
Merged
unroll byref struct copies#86820
markples merged 5 commits into
dotnet:mainfrom 
markples:w80086
Conversation
@markples
@markplesmarkples
commented
May 26, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
If a struct contains a byref, then it is known to be on the stack/regs (not in the heap), so GC write barriers are not required. This adds that case to lower*.cpp and attempts to make the code more similar. I didn't actually factor them (especially with a few subtle differences such as the call to getUnrollThreshold).
This partially handles #80086. It improves the code for common cases, but since the strategy is not always used, the correctness issue in it is not completely handled. Next step is to apply the fix for that and see how bad the regressions are; this change will reduce the impact.
Example:
staticSpan<int>Copy1(Span<int>s)=>s;
G_M44162_IG01: ;; offset=0000Hvzeroupper ;; size=3 bbWeight=1 PerfScore 1.00G_M44162_IG02: ;; offset=0003H vmovdqu xmm0, xmmword ptr [rdx] vmovdqu xmmword ptr [rcx],xmm0 ;; size=8 bbWeight=1 PerfScore 6.00G_M44162_IG03: ;; offset=000BHmovrax,rcx ;; size=3 bbWeight=1 PerfScore 0.25G_M44162_IG04: ;; offset=000EHret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code 15, prolog size 3, PerfScore 9.75, instruction count 5, allocated bytes for code 15 (MethodHash=4d5b537d) for metho
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
May 26, 2023
@ghostghost
assigned markplesMay 26, 2023
@ghost
ghost
commented
May 26, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
null
Author:markples
Assignees:markples
Labels:
area-CodeGen-coreclr
Milestone:-
@markplesmarkples
mentioned this pull request
May 26, 2023

Closed

@markplesmarkples
added this to the 8.0.0 milestone
May 26, 2023
@ghostghost
closed this
Jun 25, 2023
@ghost
ghost
commented
Jun 25, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@markplesmarkples
reopened this
Jul 15, 2023
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Diff results - lots of interference from the negative diffs hitting all PRs right now
linux arm64
Diffs are based on 876,006 contexts (105,646 MinOpts, 770,360 FullOpts).
Overall (-4,320 bytes)

MinOpts (-2,748 bytes)

FullOpts (-1,572 bytes)
linux x64
Diffs are based on 929,778 contexts (110,567 MinOpts, 819,211 FullOpts).
Overall (+1,189 bytes)

MinOpts (-422 bytes)

FullOpts (+1,611 bytes)
windows arm64
Diffs are based on 923,219 contexts (84,344 MinOpts, 838,875 FullOpts).
Overall (-4,164 bytes)

MinOpts (-2,100 bytes)

FullOpts (-2,064 bytes)
windows x64
Diffs are based on 987,198 contexts (109,803 MinOpts, 877,395 FullOpts).
Overall (-4,641 bytes)

MinOpts (-4,438 bytes)

FullOpts (-203 bytes)
linux arm
Diffs are based on 846,987 contexts (105,632 MinOpts, 741,355 FullOpts).
MISSED contexts: 33,754 (3.99%)
Overall (-10,776 bytes)

MinOpts (-7,584 bytes)

FullOpts (-3,192 bytes)
windows x86
Diffs are based on 976,498 contexts (114,341 MinOpts, 862,157 FullOpts).
Overall (+2,116 bytes)

MinOpts (+129 bytes)

FullOpts (+1,987 bytes)
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
good diff from arm64
- mov x14, x19
- ; byrRegs +[x14]
- mov x13, x20
- ; byrRegs +[x13]
- bl CORINFO_HELP_ASSIGN_BYREF
- ldr x12, [x13], #0x08
- str x12, [x14], #0x08
+ ;; size=8 bbWeight=1 PerfScore 6.00
+G_M20461_IG12: ; bbWeight=1, nogc, extend
+ ldp x2, x3, [x20]
+ stp x2, x3, [x19]
+ ;; size=8 bbWeight=1 PerfScore 5.00
+G_M20461_IG13: ; bbWeight=1, isz, extend

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
similar on x64
- add rdi, 56
- lea rsi, bword ptr [rbp-70H]
- ; byrRegs +[rsi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq + ;; size=22 bbWeight=1 PerfScore 6.50
+G_M5232_IG05: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rbp-70H]
+ vmovdqu ymmword ptr [rdi+38H], ymm0
+ ;; size=10 bbWeight=1 PerfScore 6.00

@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
occasional size regression
+3 (+5.77%) : 66460.dasm - System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
@@ -15,31 +15,34 @@
G_M50205_IG01: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
push rbp
sub rsp, 16
+ vzeroupper lea rbp, [rsp+10H]
mov bword ptr [rbp-08H], rdi
mov bword ptr [rbp-10H], rsi
- ;; size=18 bbWeight=1 PerfScore 3.75
+ ;; size=21 bbWeight=1 PerfScore 4.75
G_M50205_IG02: ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- mov rsi, bword ptr [rbp-08H]
- ; byrRegs +[rsi]
- mov rdi, bword ptr [rbp-10H]
- ; byrRegs +[rdi]
- call CORINFO_HELP_ASSIGN_BYREF
- movsq - call CORINFO_HELP_ASSIGN_BYREF
- movsq - movsq - mov rax, bword ptr [rbp-10H]
+ mov rax, bword ptr [rbp-08H]
; byrRegs +[rax]
- ;; size=28 bbWeight=1 PerfScore 8.00
-G_M50205_IG03: ; bbWeight=1, epilog, nogc, extend
+ mov rcx, bword ptr [rbp-10H]
+ ; byrRegs +[rcx]
+ ;; size=8 bbWeight=1 PerfScore 2.00
+G_M50205_IG03: ; bbWeight=1, nogc, extend
+ vmovdqu ymm0, ymmword ptr [rax]
+ vmovdqu ymmword ptr [rcx], ymm0
+ mov rdx, qword ptr [rax+20H]
+ mov qword ptr [rcx+20H], rdx
+ ;; size=16 bbWeight=1 PerfScore 10.00
+G_M50205_IG04: ; bbWeight=1, extend
+ mov rax, bword ptr [rbp-10H]
+ ;; size=4 bbWeight=1 PerfScore 1.00
+G_M50205_IG05: ; bbWeight=1, epilog, nogc, extend
add rsp, 16
pop rbp
ret ;; size=6 bbWeight=1 PerfScore 1.75
; END METHOD System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this
-; Total bytes of code 52, prolog size 10, PerfScore 18.70, instruction count 16, allocated bytes for code 52 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)
+; Total bytes of code 55, prolog size 13, PerfScore 25.00, instruction count 16, allocated bytes for code 55 (MethodHash=ee6c3be2) for method System.Text.SpanLineEnumerator:GetEnumerator():System.Text.SpanLineEnumerator:this (Tier0)

@markples
markples
marked this pull request as ready for review
July 17, 2023 19:11
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet/jit-contrib

cc @clamp03@shushanhf since there are architecture-specific changes
@markples
markples
commented
Jul 17, 2023

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Jul 17, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@clamp03
clamp03
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@dotnet/jit-contrib cc @clamp03@shushanhf since there are architecture-specific changes
Thank you! It looks good to me. CC @t-mustafin@alpencolt
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/tests/JIT/opt/Misc/Runtime_80086/Runtime_80086.csproj

Comment on lines

+1
 to 
+15

<Project Sdk="Microsoft.NET.Sdk">
 <PropertyGroup>
 <!-- Needed for CLRTestEnvironmentVariable -->
 <RequiresProcessIsolation>true</RequiresProcessIsolation>

 <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
 <Optimize>True</Optimize>
 </PropertyGroup>
 <ItemGroup>
 <Compile Include="$(MSBuildProjectName).cs" />

 <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
 <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
 </ItemGroup>
</Project>
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to set these environment variables and RequiresProcessIsolation?
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this might be my misunderstanding after seeing these set in a number (which isn't actually as many as I thought) of optimization tests. It looks like they are set when we do FileCheck asm checks (ideally we could say something like "do FileCheck X if and only if optimized" rather than "always optimize and do FileCheck X"), but I figured normal asm diffs would be sufficient here.
I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still missing something because I'd expect most tests to run without optimizations due to tiering yet most of the asmdiff contexts have optimizations enabled. Perhaps if I dug into the asmdiff setup I'd find some settings for most/all tests?
PR runs of runtime always run with tiered compilation disabled. In the rolling builds on main we run all tests both with and without tiered compilation enabled. It's set here:
runtime/eng/pipelines/common/templates/runtimes/run-test-job.yml

Lines 376 to 382
in
ac3979a
${{ elseif eq(variables['Build.Reason'], 'PullRequest') }}: 
scenarios: 
 - no_tiered_compilation
${{ else }}: 
scenarios: 
 - normal
 - no_tiered_compilation
So usually (for non file check) we will not set any environment variables and leave it up to these defined scenarios.
@markplesmarkplesJul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I hadn't found this test snippet yet.
@jakobbotsch
jakobbotsch
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay
@azure-pipelines
azure-pipelinesBot
commented
Jul 18, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
jakobbotsch
jakobbotsch
reviewed
Jul 18, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
Outdated


Comment on lines

+426
 to 
+429

 unsigned slots = this->GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
 {
 if (this->IsGCByRef(i))
@jakobbotschjakobbotschJul 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We usually don't prefix with this unless necessary.
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
cleaner results;
linux arm64
Diffs are based on 1,536,168 contexts (480,629 MinOpts, 1,055,539 FullOpts).
Overall (-5,232 bytes)

MinOpts (-3,260 bytes)

FullOpts (-1,972 bytes)
linux x64
Diffs are based on 1,660,406 contexts (558,006 MinOpts, 1,102,400 FullOpts).
Overall (-1,142 bytes)

MinOpts (-750 bytes)

FullOpts (-392 bytes)
osx arm64
Diffs are based on 1,187,983 contexts (429,171 MinOpts, 758,812 FullOpts).
Overall (-5,732 bytes)

MinOpts (-3,276 bytes)

FullOpts (-2,456 bytes)
windows arm64
Diffs are based on 1,401,929 contexts (440,568 MinOpts, 961,361 FullOpts).
Overall (-4,416 bytes)

MinOpts (-2,580 bytes)

FullOpts (-1,836 bytes)
windows x64
Diffs are based on 1,661,390 contexts (471,241 MinOpts, 1,190,149 FullOpts).
MISSED contexts: 3 (0.00%)
Overall (-8,993 bytes)

MinOpts (-5,772 bytes)

FullOpts (-3,221 bytes)
linux arm
Diffs are based on 1,323,737 contexts (358,680 MinOpts, 965,057 FullOpts).
MISSED contexts: 44,200 (3.34%)
Overall (-13,518 bytes)

MinOpts (-9,530 bytes)

FullOpts (-3,988 bytes)
windows x86
Diffs are based on 1,438,138 contexts (387,507 MinOpts, 1,050,631 FullOpts).
No diffs found.
@markples
markples
merged commit 8724933
into
dotnet:mainJul 18, 2023
@markplesmarkples
mentioned this pull request
Jul 18, 2023
 Closed

@xtqqczze
xtqqczze
commented
Jul 18, 2023

Copy link


Copy Markdown

Contributor
staticSpan<int>Copy1(Span<int>s)=>s;
Extended set of cases: https://csharp.godbolt.org/z/zf8e5qa4G
@markples
markples
commented
Jul 18, 2023

Copy link


Copy Markdown

ContributorAuthor
Thanks @xtqqczze. I think that I have all of these except for CreateSpan3 (quite similar to CreateSpan2 except with byref instead of * type).
IDisposable
IDisposable
reviewed
Jul 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/layout.cpp
bool ClassLayout::HasGCByRef() const
{
 unsigned slots = GetSlotCount();
 for (unsigned i = 0; i < slots; i++)
@IDisposableIDisposableJul 29, 2023

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.

Is there an intuition that the GCByRef slots are at the beginning or at the end or scattered? In other words, are we more likely to fast-return if we walk them list backwards?
@markplesmarkplesAug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any intuition or measurements on this. If necessary, then this could be cached similar to whether the layout has any gc pointers, though if I recall correctly we are out of bits at this size.
@markplesmarkples
mentioned this pull request
Aug 31, 2023

Closed

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


Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes

+1 more reviewer

@IDisposableIDisposableIDisposable left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@markplesmarkples

Labels

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

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@markples@clamp03@jakobbotsch@xtqqczze@IDisposable