ARM64-SVE: Allow SVE ops to re-use the same registers - #107084

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:reg3_github
Sep 13, 2024
Merged

ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:reg3_github

Conversation

@a74nh

@a74nha74nh commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

Fixes#106866

RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.

Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Aug 28, 2024
@a74nh
a74nh marked this pull request as ready for review August 28, 2024 13:52
@a74nh

a74nh commented Aug 28, 2024

Copy link
Copy Markdown
ContributorAuthor

@dotnet/arm64-contrib @kunalspathak

@a74nha74nh mentioned this pull request Aug 28, 2024
Merged
@build-analysisbuild-analysisBot mentioned this pull request Aug 28, 2024
Open
3 tasks
@a74nh

a74nh commented Aug 30, 2024

Copy link
Copy Markdown
ContributorAuthor

Given the discussions over in #107036, I think this PR is correct.

For this case we have:

Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.

@kunalspathakkunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Sep 3, 2024
@a74nha74nh mentioned this pull request Sep 9, 2024
Merged
@build-analysisbuild-analysisBot mentioned this pull request Sep 11, 2024
Closed
3 tasks
kunalspathak
kunalspathak suggested changes Sep 11, 2024
View reviewed changes

@kunalspathakkunalspathak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.

Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
}
else if (isRMW)
{
assert((targetReg == op1Reg) || (targetReg != op2Reg));

@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.

@a74nha74nhSep 13, 2024

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.

Restored this one

Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
{
if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
{
assert((targetReg == op1Reg) || (targetReg != op1Reg));

@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.

@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:

------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•
edited
Loading

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.

We need some kind of assert validating the mov is valid.

Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.

So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree

The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite

@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted

@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted

Agree.

@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert

@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.

@a74nh

a74nh commented Sep 13, 2024

Copy link
Copy Markdown
ContributorAuthor

Stress tests working with the latest version

kunalspathak
kunalspathak approved these changes Sep 13, 2024
View reviewed changes

@kunalspathakkunalspathak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@kunalspathak
kunalspathak merged commit cf9c995 into dotnet:mainSep 13, 2024
@kunalspathak

kunalspathak commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

/backport to release/9.0

@github-actions

github-actionsBot commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106

@github-actions

github-actionsBot commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions

github-actionsBot commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

kunalspathak pushed a commit to kunalspathak/runtime that referenced this pull request Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak mentioned this pull request Sep 13, 2024
Merged
4 tasks
@a74nh
a74nh deleted the reg3_github branch September 16, 2024 08:39
jeffschwMSFT added a commit that referenced this pull request Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

Assignees

No one assigned

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'

3 participants

@a74nh@kunalspathak@tannergooding
, '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
ARM64-SVE: Allow SVE ops to re-use the same registers - #107084
Merged
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Sep 13, 2024
Merged
ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Conversation
@a74nh
@a74nha74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

Contributor
Fixes#106866
RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.
Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 28, 2024
@a74nh
a74nh
marked this pull request as ready for review
August 28, 2024 13:52
@a74nh
a74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

ContributorAuthor
@dotnet/arm64-contrib @kunalspathak
@a74nha74nh
mentioned this pull request
Aug 28, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 28, 2024

Open

3 tasks
@a74nh
a74nh
commented
Aug 30, 2024

Copy link


Copy Markdown

ContributorAuthor
Given the discussions over in #107036, I think this PR is correct.
For this case we have:
Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.
@kunalspathakkunalspathak
added
the 
arm-sve
Work related to arm64 SVE/SVE2 support
label
Sep 3, 2024
@a74nha74nh
mentioned this pull request
Sep 9, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 11, 2024

Closed

3 tasks
kunalspathak
kunalspathak
suggested changes
Sep 11, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 }
 else if (isRMW)
 {
 assert((targetReg == op1Reg) || (targetReg != op2Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.
@a74nha74nhSep 13, 2024

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.

Restored this one
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 {
 if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
 {
 assert((targetReg == op1Reg) || (targetReg != op1Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.
@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:
------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•

edited

Loading

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.

We need some kind of assert validating the mov is valid.
Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.
So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree
The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite
@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
Agree.
@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert
@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.
@a74nh
a74nh
commented
Sep 13, 2024

Copy link


Copy Markdown

ContributorAuthor
Stress tests working with the latest version
kunalspathak
kunalspathak
approved these changes
Sep 13, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
@kunalspathak
kunalspathak
merged commit cf9c995
into
dotnet:mainSep 13, 2024
@kunalspathak
kunalspathak
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
/backport to release/9.0
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
kunalspathak pushed a commit
to kunalspathak/runtime
that referenced
this pull request
Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak
mentioned this pull request
Sep 13, 2024
 Merged

4 tasks
@a74nh
a74nh
deleted the

reg3_github

branch
September 16, 2024 08:39
jeffschwMSFT added a commit
that referenced
this pull request
Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

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

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'


3 participants

@a74nh@kunalspathak@tannergooding

, '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
ARM64-SVE: Allow SVE ops to re-use the same registers - #107084
Merged
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Sep 13, 2024
Merged
ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Conversation
@a74nh
@a74nha74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

Contributor
Fixes#106866
RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.
Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 28, 2024
@a74nh
a74nh
marked this pull request as ready for review
August 28, 2024 13:52
@a74nh
a74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

ContributorAuthor
@dotnet/arm64-contrib @kunalspathak
@a74nha74nh
mentioned this pull request
Aug 28, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 28, 2024

Open

3 tasks
@a74nh
a74nh
commented
Aug 30, 2024

Copy link


Copy Markdown

ContributorAuthor
Given the discussions over in #107036, I think this PR is correct.
For this case we have:
Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.
@kunalspathakkunalspathak
added
the 
arm-sve
Work related to arm64 SVE/SVE2 support
label
Sep 3, 2024
@a74nha74nh
mentioned this pull request
Sep 9, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 11, 2024

Closed

3 tasks
kunalspathak
kunalspathak
suggested changes
Sep 11, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 }
 else if (isRMW)
 {
 assert((targetReg == op1Reg) || (targetReg != op2Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.
@a74nha74nhSep 13, 2024

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.

Restored this one
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 {
 if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
 {
 assert((targetReg == op1Reg) || (targetReg != op1Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.
@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:
------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•

edited

Loading

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.

We need some kind of assert validating the mov is valid.
Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.
So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree
The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite
@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
Agree.
@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert
@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.
@a74nh
a74nh
commented
Sep 13, 2024

Copy link


Copy Markdown

ContributorAuthor
Stress tests working with the latest version
kunalspathak
kunalspathak
approved these changes
Sep 13, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
@kunalspathak
kunalspathak
merged commit cf9c995
into
dotnet:mainSep 13, 2024
@kunalspathak
kunalspathak
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
/backport to release/9.0
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
kunalspathak pushed a commit
to kunalspathak/runtime
that referenced
this pull request
Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak
mentioned this pull request
Sep 13, 2024
 Merged

4 tasks
@a74nh
a74nh
deleted the

reg3_github

branch
September 16, 2024 08:39
jeffschwMSFT added a commit
that referenced
this pull request
Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

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

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'


3 participants

@a74nh@kunalspathak@tannergooding

, '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
ARM64-SVE: Allow SVE ops to re-use the same registers - #107084
Merged
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Sep 13, 2024
Merged
ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Conversation
@a74nh
@a74nha74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

Contributor
Fixes#106866
RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.
Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 28, 2024
@a74nh
a74nh
marked this pull request as ready for review
August 28, 2024 13:52
@a74nh
a74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

ContributorAuthor
@dotnet/arm64-contrib @kunalspathak
@a74nha74nh
mentioned this pull request
Aug 28, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 28, 2024

Open

3 tasks
@a74nh
a74nh
commented
Aug 30, 2024

Copy link


Copy Markdown

ContributorAuthor
Given the discussions over in #107036, I think this PR is correct.
For this case we have:
Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.
@kunalspathakkunalspathak
added
the 
arm-sve
Work related to arm64 SVE/SVE2 support
label
Sep 3, 2024
@a74nha74nh
mentioned this pull request
Sep 9, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 11, 2024

Closed

3 tasks
kunalspathak
kunalspathak
suggested changes
Sep 11, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 }
 else if (isRMW)
 {
 assert((targetReg == op1Reg) || (targetReg != op2Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.
@a74nha74nhSep 13, 2024

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.

Restored this one
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 {
 if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
 {
 assert((targetReg == op1Reg) || (targetReg != op1Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.
@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:
------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•

edited

Loading

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.

We need some kind of assert validating the mov is valid.
Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.
So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree
The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite
@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
Agree.
@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert
@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.
@a74nh
a74nh
commented
Sep 13, 2024

Copy link


Copy Markdown

ContributorAuthor
Stress tests working with the latest version
kunalspathak
kunalspathak
approved these changes
Sep 13, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
@kunalspathak
kunalspathak
merged commit cf9c995
into
dotnet:mainSep 13, 2024
@kunalspathak
kunalspathak
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
/backport to release/9.0
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
kunalspathak pushed a commit
to kunalspathak/runtime
that referenced
this pull request
Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak
mentioned this pull request
Sep 13, 2024
 Merged

4 tasks
@a74nh
a74nh
deleted the

reg3_github

branch
September 16, 2024 08:39
jeffschwMSFT added a commit
that referenced
this pull request
Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

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

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'


3 participants

@a74nh@kunalspathak@tannergooding

, '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
ARM64-SVE: Allow SVE ops to re-use the same registers - #107084
Merged
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Sep 13, 2024
Merged
ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Conversation
@a74nh
@a74nha74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

Contributor
Fixes#106866
RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.
Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 28, 2024
@a74nh
a74nh
marked this pull request as ready for review
August 28, 2024 13:52
@a74nh
a74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

ContributorAuthor
@dotnet/arm64-contrib @kunalspathak
@a74nha74nh
mentioned this pull request
Aug 28, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 28, 2024

Open

3 tasks
@a74nh
a74nh
commented
Aug 30, 2024

Copy link


Copy Markdown

ContributorAuthor
Given the discussions over in #107036, I think this PR is correct.
For this case we have:
Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.
@kunalspathakkunalspathak
added
the 
arm-sve
Work related to arm64 SVE/SVE2 support
label
Sep 3, 2024
@a74nha74nh
mentioned this pull request
Sep 9, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 11, 2024

Closed

3 tasks
kunalspathak
kunalspathak
suggested changes
Sep 11, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 }
 else if (isRMW)
 {
 assert((targetReg == op1Reg) || (targetReg != op2Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.
@a74nha74nhSep 13, 2024

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.

Restored this one
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 {
 if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
 {
 assert((targetReg == op1Reg) || (targetReg != op1Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.
@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:
------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•

edited

Loading

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.

We need some kind of assert validating the mov is valid.
Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.
So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree
The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite
@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
Agree.
@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert
@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.
@a74nh
a74nh
commented
Sep 13, 2024

Copy link


Copy Markdown

ContributorAuthor
Stress tests working with the latest version
kunalspathak
kunalspathak
approved these changes
Sep 13, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
@kunalspathak
kunalspathak
merged commit cf9c995
into
dotnet:mainSep 13, 2024
@kunalspathak
kunalspathak
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
/backport to release/9.0
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
kunalspathak pushed a commit
to kunalspathak/runtime
that referenced
this pull request
Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak
mentioned this pull request
Sep 13, 2024
 Merged

4 tasks
@a74nh
a74nh
deleted the

reg3_github

branch
September 16, 2024 08:39
jeffschwMSFT added a commit
that referenced
this pull request
Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

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

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'


3 participants

@a74nh@kunalspathak@tannergooding

, '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
ARM64-SVE: Allow SVE ops to re-use the same registers - #107084
Merged
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Sep 13, 2024
Merged
ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Conversation
@a74nh
@a74nha74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

Contributor
Fixes#106866
RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.
Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 28, 2024
@a74nh
a74nh
marked this pull request as ready for review
August 28, 2024 13:52
@a74nh
a74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

ContributorAuthor
@dotnet/arm64-contrib @kunalspathak
@a74nha74nh
mentioned this pull request
Aug 28, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 28, 2024

Open

3 tasks
@a74nh
a74nh
commented
Aug 30, 2024

Copy link


Copy Markdown

ContributorAuthor
Given the discussions over in #107036, I think this PR is correct.
For this case we have:
Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.
@kunalspathakkunalspathak
added
the 
arm-sve
Work related to arm64 SVE/SVE2 support
label
Sep 3, 2024
@a74nha74nh
mentioned this pull request
Sep 9, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 11, 2024

Closed

3 tasks
kunalspathak
kunalspathak
suggested changes
Sep 11, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 }
 else if (isRMW)
 {
 assert((targetReg == op1Reg) || (targetReg != op2Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.
@a74nha74nhSep 13, 2024

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.

Restored this one
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 {
 if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
 {
 assert((targetReg == op1Reg) || (targetReg != op1Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.
@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:
------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•

edited

Loading

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.

We need some kind of assert validating the mov is valid.
Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.
So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree
The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite
@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
Agree.
@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert
@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.
@a74nh
a74nh
commented
Sep 13, 2024

Copy link


Copy Markdown

ContributorAuthor
Stress tests working with the latest version
kunalspathak
kunalspathak
approved these changes
Sep 13, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
@kunalspathak
kunalspathak
merged commit cf9c995
into
dotnet:mainSep 13, 2024
@kunalspathak
kunalspathak
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
/backport to release/9.0
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
kunalspathak pushed a commit
to kunalspathak/runtime
that referenced
this pull request
Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak
mentioned this pull request
Sep 13, 2024
 Merged

4 tasks
@a74nh
a74nh
deleted the

reg3_github

branch
September 16, 2024 08:39
jeffschwMSFT added a commit
that referenced
this pull request
Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

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

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'


3 participants

@a74nh@kunalspathak@tannergooding

, '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
ARM64-SVE: Allow SVE ops to re-use the same registers - #107084
Merged
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Sep 13, 2024
Merged
ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Conversation
@a74nh
@a74nha74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

Contributor
Fixes#106866
RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.
Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 28, 2024
@a74nh
a74nh
marked this pull request as ready for review
August 28, 2024 13:52
@a74nh
a74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

ContributorAuthor
@dotnet/arm64-contrib @kunalspathak
@a74nha74nh
mentioned this pull request
Aug 28, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 28, 2024

Open

3 tasks
@a74nh
a74nh
commented
Aug 30, 2024

Copy link


Copy Markdown

ContributorAuthor
Given the discussions over in #107036, I think this PR is correct.
For this case we have:
Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.
@kunalspathakkunalspathak
added
the 
arm-sve
Work related to arm64 SVE/SVE2 support
label
Sep 3, 2024
@a74nha74nh
mentioned this pull request
Sep 9, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 11, 2024

Closed

3 tasks
kunalspathak
kunalspathak
suggested changes
Sep 11, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 }
 else if (isRMW)
 {
 assert((targetReg == op1Reg) || (targetReg != op2Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.
@a74nha74nhSep 13, 2024

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.

Restored this one
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 {
 if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
 {
 assert((targetReg == op1Reg) || (targetReg != op1Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.
@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:
------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•

edited

Loading

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.

We need some kind of assert validating the mov is valid.
Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.
So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree
The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite
@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
Agree.
@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert
@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.
@a74nh
a74nh
commented
Sep 13, 2024

Copy link


Copy Markdown

ContributorAuthor
Stress tests working with the latest version
kunalspathak
kunalspathak
approved these changes
Sep 13, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
@kunalspathak
kunalspathak
merged commit cf9c995
into
dotnet:mainSep 13, 2024
@kunalspathak
kunalspathak
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
/backport to release/9.0
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
kunalspathak pushed a commit
to kunalspathak/runtime
that referenced
this pull request
Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak
mentioned this pull request
Sep 13, 2024
 Merged

4 tasks
@a74nh
a74nh
deleted the

reg3_github

branch
September 16, 2024 08:39
jeffschwMSFT added a commit
that referenced
this pull request
Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

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

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'


3 participants

@a74nh@kunalspathak@tannergooding

, '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
ARM64-SVE: Allow SVE ops to re-use the same registers - #107084
Merged
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Sep 13, 2024
Merged
ARM64-SVE: Allow SVE ops to re-use the same registers#107084
kunalspathak merged 5 commits into
dotnet:mainfrom 
a74nh:reg3_github
Conversation
@a74nh
@a74nha74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

Contributor
Fixes#106866
RMW intrinsics can use the RMW register in other inputs. The only restriction is that those inputs will be overwritten.
Added extra intrinsic testing to cover this. Requires #107036 for all the tests to pass.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 28, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 28, 2024
@a74nh
a74nh
marked this pull request as ready for review
August 28, 2024 13:52
@a74nh
a74nh
commented
Aug 28, 2024

Copy link


Copy Markdown

ContributorAuthor
@dotnet/arm64-contrib @kunalspathak
@a74nha74nh
mentioned this pull request
Aug 28, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 28, 2024

Open

3 tasks
@a74nh
a74nh
commented
Aug 30, 2024

Copy link


Copy Markdown

ContributorAuthor
Given the discussions over in #107036, I think this PR is correct.
For this case we have:
Generating: N017 ( 1, 1) [000033] -----+----- t33 = LCL_VAR simd16 V07 tmp6 u:2 d0 REG d0 $80
Generating: N019 ( 1, 1) [000034] -----+----- t34 = LCL_VAR simd16 V07 tmp6 u:2 d0 (last use) REG d0 $80
/--* t37 mask
+--* t33 simd16
+--* t34 simd16
Generating: N021 ( 6, 6) [000035] -----+----- t35 = * HWINTRINSIC simd16 byte Splice REG d0 $240
V07 in reg d0 is becoming dead [000034]
Live regs: 0000000100000000 {d0} - {d0} => 0000000000000000 {}
Live vars after [000034]: {V07} -{V07} => {}
IN0004: splice z0.b, p0, z0.b, z0.b

Like in the previous PR, we have the same value used twice. But here we are not going to use a MOVPRFX (due to it not being wrapped with conditional select). Therefore we have no requirement to delay free the operands, and can simply allow the same one to be used.
@kunalspathakkunalspathak
added
the 
arm-sve
Work related to arm64 SVE/SVE2 support
label
Sep 3, 2024
@a74nha74nh
mentioned this pull request
Sep 9, 2024
 Merged

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 11, 2024

Closed

3 tasks
kunalspathak
kunalspathak
suggested changes
Sep 11, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I am not totally convinced that we should be removing all the asserts without seeing a case that hits it. Its ok to put some of them back for now and relax them as we see cases.
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 }
 else if (isRMW)
 {
 assert((targetReg == op1Reg) || (targetReg != op2Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

as we spoke offline, did you verify if we put this assert back, do we hit them. These asserts were added conservatively with the idea that we understand the scenario under which this cannot be true. I am wondering if we should remove these as we see scenarios that trigger them.
@a74nha74nhSep 13, 2024

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.

Restored this one
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
Comment threadsrc/coreclr/jit/hwintrinsiccodegenarm64.cpp
 {
 if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
 {
 assert((targetReg == op1Reg) || (targetReg != op1Reg));
@kunalspathakkunalspathakSep 11, 2024

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.

these are OK to have.
@a74nha74nhSep 13, 2024

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.

These ones are hit by the new hwintrinsic tests:
------------------- {'JitStressRegs': '1'} -------------------
Errors:
Assert failure(PID 366410 [0x0005974a], Thread: 366410 [0x5974a]): Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' in 'JIT.HardwareIntrinsics.Arm._Sve.SimpleTernaryOpTest__Sve_ConditionalExtractAfterLastActiveElementAndReplicate_float:RunSameClassFldScenario():this' during 'Generate code' (IL size 92; hash 0xa42df59e; FullOpts)
File: /home/alahay01/dotnet/runtime_sve_api/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:1118
Image: /home/alahay01/dotnet/runtime_sve_api/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun

@tannergoodingtannergoodingSep 13, 2024•

edited

Loading

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.

We need some kind of assert validating the mov is valid.
Since we're doing mov targetReg, op2Reg, then when targetReg == op1Reg and op1Reg != op2Reg, then the we would overwrite op1 and produce in incorrect results.
So, therefore, we expect that targetReg == op2Reg so no mov is emitted -or- op1/op2 have been marked delayFree
The assert therefore needs to be something like assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)). Which validates no move or no overwrite
@tannergoodingtannergoodingSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
@kunalspathakkunalspathakSep 13, 2024

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.

All paths need some kind of similar assert validating no move or no overwrite, if a move is emitted
Agree.
@a74nha74nhSep 13, 2024

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.

Done replaced this assert as suggested, and the next one with a similar assert
@a74nha74nhSep 13, 2024

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.

confirmed stress tests passing.
@a74nh
a74nh
commented
Sep 13, 2024

Copy link


Copy Markdown

ContributorAuthor
Stress tests working with the latest version
kunalspathak
kunalspathak
approved these changes
Sep 13, 2024
View reviewed changes
@kunalspathakkunalspathak
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
@kunalspathak
kunalspathak
merged commit cf9c995
into
dotnet:mainSep 13, 2024
@kunalspathak
kunalspathak
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
/backport to release/9.0
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10855965106
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak backporting to release/9.0 failed, the patch most likely resulted in conflicts:
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: ARM64-SVE: Allow SVE ops to re-use the same registers
Using index info to reconstruct a base tree...
M	src/coreclr/jit/hwintrinsiccodegenarm64.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/hwintrinsiccodegenarm64.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/hwintrinsiccodegenarm64.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 ARM64-SVE: Allow SVE ops to re-use the same registers
Error: The process '/usr/bin/git' failed with exit code 128
Please backport manually!
@github-actions
github-actionsBot
commented
Sep 13, 2024

Copy link


Copy Markdown

Contributor
@kunalspathak an error occurred while backporting to release/9.0, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
kunalspathak pushed a commit
to kunalspathak/runtime
that referenced
this pull request
Sep 13, 2024
@a74nh@kunalspathak
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
0ac6b99
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@kunalspathakkunalspathak
mentioned this pull request
Sep 13, 2024
 Merged

4 tasks
@a74nh
a74nh
deleted the

reg3_github

branch
September 16, 2024 08:39
jeffschwMSFT added a commit
that referenced
this pull request
Sep 16, 2024
@kunalspathak@a74nh@jeffschwMSFT
[release/9.0]: ARM64-SVE: Allow SVE ops to re-use the same registers (#…
e256321
…107818)
* ARM64-SVE: Allow SVE ops to re-use the same registers (#107084)
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
* resolve merge conflicts
---------
Co-authored-by: Alan Hayward <a74nh@users.noreply.github.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@a74nh@jtschuster
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
c039377
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@a74nh@sirntar
ARM64-SVE: Allow SVE ops to re-use the same registers (dotnet#107084)
412ac21
* ARM64-SVE: Allow SVE ops to re-use the same registers
* Add Sve.IsSupported
* restore an assert
* better asserts
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 16, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding left review comments

+1 more reviewer

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

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIarm-sveWork related to arm64 SVE/SVE2 supportcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT SVE: Assertion failed '(targetReg == op1Reg) || (targetReg != op3Reg)' during 'Generate code'


3 participants

@a74nh@kunalspathak@tannergooding