JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall
, '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

JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall
, '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

JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall
, '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

JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall
, '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

JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall
, '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

JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall
, '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

JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall
, '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

JIT: Unblock Vector###<long> intrinsics on x86 - #112728

Merged
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64
Mar 20, 2025
Merged

JIT: Unblock Vector###<long> intrinsics on x86#112728
BruceForstall merged 20 commits into
dotnet:mainfrom
saucecontrol:createscalar64

Conversation

@saucecontrol

@saucecontrolsaucecontrol commented Feb 20, 2025

Copy link
Copy Markdown
Member

Resolves#11626

This resolves a large number of TODOs around HWIntrinsic expansion involving scalar longs on x86.

The most significant change here is in promoting CreateScalar and ToScalar to be code generating intrinsics instead of converting them to other intrinsics at lowering. This was necessary in order to handle emitting movq for scalar long loads/stores but also unlocks several other optimizations since we can now allow CreateScalar and ToScalar to be contained and can specialize codegen depending on whether they end up loading/storing from/to memory or not. Some example improvements on x64:

Vector128.CreateScalar(ref float):

- vinsertps xmm0, xmm0, dword ptr [rbp+0x10], 14+ vmovss xmm0, dword ptr [rbp+0x10]

Vector128.CreateScalar(ref double):

- vxorps xmm0, xmm0, xmm0- vmovsd xmm1, qword ptr [rbp-0x08]- vmovsd xmm0, xmm0, xmm1+ vmovsd xmm0, qword ptr [rbp-0x08]

ref byte = Vector128<byte>.ToScalar():

- vmovd r9d, xmm3- mov byte ptr [r10], r9b+ vpextrb byte ptr [r10], xmm3, 0

Vector<byte>.ToScalar()

- vmovups ymm0, ymmword ptr [esp+0x04]- vmovd eax, xmm0- movzx eax, al+ movzx eax, byte ptr [esp+0x04]

And the less realistic, but still interesting
Sse.AddScalar(Vector128.CreateScalar(ref float), Vector128.CreateScalar(ref float)).ToScalar():

- xorps xmm0, xmm0- movss xmm1, dword ptr [rcx]- movss xmm0, xmm1- xorps xmm1, xmm1- movss xmm2, dword ptr [rdx]- movss xmm1, xmm2- addss xmm0, xmm1+ movss xmm0, dword ptr [rcx]+ addss xmm0, dword ptr [rdx]

This also removes some redundant casts for CreateScalar of small types. Previously, a zero-extending cast was inserted unconditionally and was sometimes removed by peephole opt on x64 but often wasn't.

Vector128.CreateScalar(short):

- movsx rax, dx- movzx rax, ax- movd xmm0, rax+ movzx rax, dx+ movd xmm0, eax

Vector128.CreateScalar(checked((byte)val)):

 cmp edx, 255
ja SHORT G_M000_IG04
mov eax, edx
- movzx rax, al- vmovd xmm0, rax+ vmovd xmm0, eax

Vector128.CreateScalar(ref sbyte):

- movsx rax, byte ptr [rdx]- movzx rax, al- vmovd xmm0, rax+ movzx rax, byte ptr [rdx]+ vmovd xmm0, eax

x86 diffs are much more significant, because of the newly-enabled intrinsic expansion:

CollectionBase size (bytes)Diff size (bytes)PerfScore in Diffs
benchmarks.run.windows.x86.checked.mch7,149,204-1,892-2.17%
benchmarks.run_pgo.windows.x86.checked.mch46,986,713-738+0.03%
benchmarks.run_tiered.windows.x86.checked.mch9,470,045-976+0.11%
coreclr_tests.run.windows.x86.checked.mch320,065,247-205,564-6.41%
libraries.crossgen2.windows.x86.checked.mch31,314,339-15,854-4.11%
libraries.pmi.windows.x86.checked.mch34,326,245-14,416-2.19%
libraries_tests.run.windows.x86.Release.mch215,517,600-55,366-2.41%
libraries_tests_no_tiered_compilation.run.windows.x86.Release.mch115,783,488-80,576-3.65%
realworld.run.windows.x86.checked.mch9,587,950-467-0.45%

@saucecontrolsaucecontrol left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is ready for review.
cc @tannergooding

Comment on lines -489 to -504
// Keep casts with operands usable from memory.
if (castOp->isContained() || castOp->IsRegOptional())
{
return op;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This condition, added in #72719, made this method effectively useless. Removing it was a zero-diff change. I can look in future at containing the casts rather than removing them.


GenTree* op2 = node->Op(2);

// TODO-XArch-AVX512 : Merge the NI_Vector512_Create and NI_Vector256_Create paths below.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The churn in this section is just taking care of this TODO

tmp2 = InsertNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, simdBaseJitType, 16);
LowerNode(tmp2);

node->ResetHWIntrinsicId(NI_SSE_MoveLowToHigh, tmp1, tmp2);

@saucecontrolsaucecontrolFeb 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changing this to UnpackLow shows up as a regression in a few places, because movlhps is one byte smaller, but it enables other optimizations since unpcklpd takes a memory operand plus mask and embedded broadcast.

Vector128.Create(double, 1.0):

- vmovups xmm0, xmmword ptr [reloc @RWD00]- vmovlhps xmm0, xmm1, xmm0+ vunpcklpd xmm0, xmm1, qword ptr [reloc @RWD00] {1to2}

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.

This should probably be peepholed back to vmovlhps if both are from register.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was thinking the same but would rather save that for a followup. llvm has a replacement list of equivalent instructions that have different sizes, and unpcklpd is on it, as are things like vpermilps, which is replaced by pshufd.

It's worth having a discussion about whether we'd also want to do replacements that switch between float and integer domains. I'll open an issue.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I looked at this again, and there's actually only a size difference for legacy SSE encoding, so it's probably not worth special casing.

Comment on lines +2391 to +2395
if (varDsc->lvIsParam)
{
// Promotion blocks combined read optimizations for SIMD loads of long params
return;
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In isolation, this change produced a small number of diffs and was mostly an improvement. A few regressions show up in the SPMI reports, but the overall impact is good, especially considering the places we can load a long to vector with movq

@saucecontrol
saucecontrol marked this pull request as ready for review February 22, 2025 00:18
@saucecontrol

Copy link
Copy Markdown
MemberAuthor

It occurred to me the optimization to emit pinsrb/w for CreateScalarUnsafe was a bad idea because it creates a false dependency on the upper bits of the target reg. Removed that.

Comment threadsrc/coreclr/jit/decomposelongs.cpp
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/decomposelongs.cpp Outdated

@tannergoodingtannergooding left a comment

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.

CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate emitting movq for the CreateScalarUnsafe helper intrinsics that take a long/ulong on x86

4 participants

@saucecontrol@jakobbotsch@tannergooding@BruceForstall