JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

JIT ARM64-SVE: Add AddAcross - #101674

Merged
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github
May 1, 2024
Merged

JIT ARM64-SVE: Add AddAcross#101674
kunalspathak merged 5 commits into
dotnet:mainfrom
a74nh:sve_addacross_github

Conversation

@a74nh

Copy link
Copy Markdown
Contributor

Contributes towards #99957

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 29, 2024
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.


// IF_SVE_AI_3A
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_1BYTE, REG_V1, REG_P4, REG_V2,
theEmitter->emitIns_R_R_R(INS_sve_saddv, EA_SCALABLE, REG_V1, REG_P4, REG_V2,

@a74nha74nhApr 29, 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.

All the codegen changes:

For these instructions, arg2 (EA_1BYTE etc) is never used as the return value is dependent on the input type which is already specified in opt.
Switching arg2 to EA_SCALABLE means there is no need to write special hwinstrinsiccodegen code.

I've changed the bare minimal of instructions needed to make this patch work. There are quite a few more reduction like instructions - we should do those as we get to them in the API

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

This file is a copy of _SveUnaryOpTestTemplate.template with conditional tests removed as they can't be used for reduction.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding

@a74nh
a74nhforce-pushed the sve_addacross_github branch 2 times, most recently from a0aaade to df9385bCompareApril 29, 2024 11:17
@a74nh
a74nhforce-pushed the sve_addacross_github branch from df9385b to a3d0161CompareApril 29, 2024 12:22
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@a74nh
a74nh marked this pull request as ready for review April 29, 2024 12:56
@a74nh

Copy link
Copy Markdown
ContributorAuthor

@kunalspathak @dotnet/arm64-contrib

@kunalspathak

Copy link
Copy Markdown
Contributor

superpmi-* failures are #101685

@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.

added some questions and minor changes.

Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated
@@ -0,0 +1,302 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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.

We can do cndSel(mask, AddAcross(a), falseVal). The result of this would be:

  • If 0th lane was active in mask, it would have the result of AddAcross(a)
  • For all the other lanes, it will either have 0 (for active lanes) or falseVal for in-active lanes

If that makes sense, can you add those in your new template? This might not be super meaningful but want to make sure that we test the jit code paths at least.
cc: @tannergooding


public static ushort AddWidening(ushort op1, byte op2) => (ushort)(op1 + op2);

public static ulong AddWidening(ulong op1, byte op2) => (ulong)(op1 + (ulong)op2);

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.

do we also not need the following?

 public static uint AddWidening(uint op1, byte op2) => (uint)(op1 + (uint)op2);

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.

Not for the testing. Sve.AddAcross() always widens to 64bits regardless of the input.

helper.cs seems to be taking the approach of only adding helper functions when they are needed.

return reduceOp(op1[0], op1[1]);
}

if (op1.Length % 2 != 0)

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.

do we have input parameters that exercise this condition?

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.

No. This would require an vector where the number of elements was not a power of 2.

I wasn't sure if there was a way of raising an exception in the testing. So instead, NaN would ensure the test failed.

@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

Copy link
Copy Markdown
Contributor

/ba-g Failure is #101721

@kunalspathak
kunalspathak merged commit a700005 into dotnet:mainMay 1, 2024
@a74nh
a74nh deleted the sve_addacross_github branch May 1, 2024 08:11
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add AddAcross
* Remove enum changes
* Fix SVE tests max vector size to 512bit
* fix zip test cases
---------
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@a74nh@kunalspathak