Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527

Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom
neon-sunset:fix-ascii-arm64
Closed

Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom
neon-sunset:fix-ascii-arm64

Conversation

@neon-sunset

@neon-sunsetneon-sunset commented Aug 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

@ghostghost added area-System.Text.Encoding community-contribution Indicates that the PR has been added by a community member labels Aug 14, 2023
@ghost

ghost commented Aug 14, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #89924

Author:neon-sunset
Assignees:-
Labels:

area-System.Text.Encoding

Milestone:-

stephentoub
stephentoub reviewed Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
if (!Vector512.IsHardwareAccelerated &&
!Vector256.IsHardwareAccelerated &&
(Sse2.IsSupported || AdvSimd.IsSupported))
if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)

@stephentoubstephentoubAug 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.

But I'd also explicitly asked about this:
#88532 (comment)
and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?

cc: @tannergooding, @anthonycanino

@neon-sunsetneon-sunsetAug 14, 2023•
edited
Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.

Let me fix that.

#88532 (comment)

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.

@tannergoodingtannergoodingAug 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91

The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.

This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?

@neon-sunsetneon-sunsetAug 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91

You are correct, my mistake.

This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?

This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄

@stephentoubstephentoub mentioned this pull request Aug 14, 2023
Open
@build-analysisbuild-analysisBot mentioned this pull request Aug 14, 2023
Closed
@build-analysisbuild-analysisBot mentioned this pull request Oct 26, 2023
Closed
@neon-sunset
neon-sunset marked this pull request as draft October 26, 2023 10:52
This was referenced Nov 7, 2023
Closed
Closed
Closed
@ghostghost closed this Dec 7, 2023
@ghost

ghost commented Dec 7, 2023

Copy link
Copy Markdown

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

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jan 7, 2024
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1

5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh
, '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
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527
Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Closed
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Conversation
@neon-sunset
@neon-sunsetneon-sunset
commented
Aug 14, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@ghostghost
added

area-System.Text.Encoding

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 14, 2023
@ghost
ghost
commented
Aug 14, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89924
Author:neon-sunset
Assignees:-
Labels:
area-System.Text.Encoding
Milestone:-
stephentoub
stephentoub
reviewed
Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
 if (!Vector512.IsHardwareAccelerated &&
 !Vector256.IsHardwareAccelerated &&
 (Sse2.IsSupported || AdvSimd.IsSupported))
 if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)
@stephentoubstephentoubAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
But I'd also explicitly asked about this:
#88532 (comment)

and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?
cc: @tannergooding, @anthonycanino
@neon-sunsetneon-sunsetAug 14, 2023•

edited

Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
Let me fix that.
#88532 (comment)
The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@tannergoodingtannergoodingAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
@neon-sunsetneon-sunsetAug 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
You are correct, my mistake.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄
@stephentoubstephentoub
mentioned this pull request
Aug 14, 2023

Open

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 14, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Oct 26, 2023

Closed

@neon-sunset
neon-sunset
marked this pull request as draft
October 26, 2023 10:52

This was referenced Nov 7, 2023

Closed


Closed


Closed

@ghostghost
closed this
Dec 7, 2023
@ghost
ghost
commented
Dec 7, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 7, 2024

This pull request was closed.

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


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees


No one assigned


Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1


5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh

, '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
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527
Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Closed
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Conversation
@neon-sunset
@neon-sunsetneon-sunset
commented
Aug 14, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@ghostghost
added

area-System.Text.Encoding

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 14, 2023
@ghost
ghost
commented
Aug 14, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89924
Author:neon-sunset
Assignees:-
Labels:
area-System.Text.Encoding
Milestone:-
stephentoub
stephentoub
reviewed
Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
 if (!Vector512.IsHardwareAccelerated &&
 !Vector256.IsHardwareAccelerated &&
 (Sse2.IsSupported || AdvSimd.IsSupported))
 if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)
@stephentoubstephentoubAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
But I'd also explicitly asked about this:
#88532 (comment)

and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?
cc: @tannergooding, @anthonycanino
@neon-sunsetneon-sunsetAug 14, 2023•

edited

Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
Let me fix that.
#88532 (comment)
The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@tannergoodingtannergoodingAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
@neon-sunsetneon-sunsetAug 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
You are correct, my mistake.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄
@stephentoubstephentoub
mentioned this pull request
Aug 14, 2023

Open

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 14, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Oct 26, 2023

Closed

@neon-sunset
neon-sunset
marked this pull request as draft
October 26, 2023 10:52

This was referenced Nov 7, 2023

Closed


Closed


Closed

@ghostghost
closed this
Dec 7, 2023
@ghost
ghost
commented
Dec 7, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 7, 2024

This pull request was closed.

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


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees


No one assigned


Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1


5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh

, '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
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527
Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Closed
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Conversation
@neon-sunset
@neon-sunsetneon-sunset
commented
Aug 14, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@ghostghost
added

area-System.Text.Encoding

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 14, 2023
@ghost
ghost
commented
Aug 14, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89924
Author:neon-sunset
Assignees:-
Labels:
area-System.Text.Encoding
Milestone:-
stephentoub
stephentoub
reviewed
Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
 if (!Vector512.IsHardwareAccelerated &&
 !Vector256.IsHardwareAccelerated &&
 (Sse2.IsSupported || AdvSimd.IsSupported))
 if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)
@stephentoubstephentoubAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
But I'd also explicitly asked about this:
#88532 (comment)

and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?
cc: @tannergooding, @anthonycanino
@neon-sunsetneon-sunsetAug 14, 2023•

edited

Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
Let me fix that.
#88532 (comment)
The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@tannergoodingtannergoodingAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
@neon-sunsetneon-sunsetAug 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
You are correct, my mistake.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄
@stephentoubstephentoub
mentioned this pull request
Aug 14, 2023

Open

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 14, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Oct 26, 2023

Closed

@neon-sunset
neon-sunset
marked this pull request as draft
October 26, 2023 10:52

This was referenced Nov 7, 2023

Closed


Closed


Closed

@ghostghost
closed this
Dec 7, 2023
@ghost
ghost
commented
Dec 7, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 7, 2024

This pull request was closed.

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


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees


No one assigned


Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1


5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh

, '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
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527
Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Closed
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Conversation
@neon-sunset
@neon-sunsetneon-sunset
commented
Aug 14, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@ghostghost
added

area-System.Text.Encoding

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 14, 2023
@ghost
ghost
commented
Aug 14, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89924
Author:neon-sunset
Assignees:-
Labels:
area-System.Text.Encoding
Milestone:-
stephentoub
stephentoub
reviewed
Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
 if (!Vector512.IsHardwareAccelerated &&
 !Vector256.IsHardwareAccelerated &&
 (Sse2.IsSupported || AdvSimd.IsSupported))
 if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)
@stephentoubstephentoubAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
But I'd also explicitly asked about this:
#88532 (comment)

and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?
cc: @tannergooding, @anthonycanino
@neon-sunsetneon-sunsetAug 14, 2023•

edited

Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
Let me fix that.
#88532 (comment)
The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@tannergoodingtannergoodingAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
@neon-sunsetneon-sunsetAug 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
You are correct, my mistake.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄
@stephentoubstephentoub
mentioned this pull request
Aug 14, 2023

Open

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 14, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Oct 26, 2023

Closed

@neon-sunset
neon-sunset
marked this pull request as draft
October 26, 2023 10:52

This was referenced Nov 7, 2023

Closed


Closed


Closed

@ghostghost
closed this
Dec 7, 2023
@ghost
ghost
commented
Dec 7, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 7, 2024

This pull request was closed.

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


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees


No one assigned


Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1


5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh

, '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
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527
Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Closed
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Conversation
@neon-sunset
@neon-sunsetneon-sunset
commented
Aug 14, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@ghostghost
added

area-System.Text.Encoding

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 14, 2023
@ghost
ghost
commented
Aug 14, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89924
Author:neon-sunset
Assignees:-
Labels:
area-System.Text.Encoding
Milestone:-
stephentoub
stephentoub
reviewed
Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
 if (!Vector512.IsHardwareAccelerated &&
 !Vector256.IsHardwareAccelerated &&
 (Sse2.IsSupported || AdvSimd.IsSupported))
 if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)
@stephentoubstephentoubAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
But I'd also explicitly asked about this:
#88532 (comment)

and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?
cc: @tannergooding, @anthonycanino
@neon-sunsetneon-sunsetAug 14, 2023•

edited

Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
Let me fix that.
#88532 (comment)
The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@tannergoodingtannergoodingAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
@neon-sunsetneon-sunsetAug 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
You are correct, my mistake.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄
@stephentoubstephentoub
mentioned this pull request
Aug 14, 2023

Open

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 14, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Oct 26, 2023

Closed

@neon-sunset
neon-sunset
marked this pull request as draft
October 26, 2023 10:52

This was referenced Nov 7, 2023

Closed


Closed


Closed

@ghostghost
closed this
Dec 7, 2023
@ghost
ghost
commented
Dec 7, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 7, 2024

This pull request was closed.

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


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees


No one assigned


Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1


5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh

, '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
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527
Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Closed
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Conversation
@neon-sunset
@neon-sunsetneon-sunset
commented
Aug 14, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@ghostghost
added

area-System.Text.Encoding

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 14, 2023
@ghost
ghost
commented
Aug 14, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89924
Author:neon-sunset
Assignees:-
Labels:
area-System.Text.Encoding
Milestone:-
stephentoub
stephentoub
reviewed
Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
 if (!Vector512.IsHardwareAccelerated &&
 !Vector256.IsHardwareAccelerated &&
 (Sse2.IsSupported || AdvSimd.IsSupported))
 if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)
@stephentoubstephentoubAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
But I'd also explicitly asked about this:
#88532 (comment)

and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?
cc: @tannergooding, @anthonycanino
@neon-sunsetneon-sunsetAug 14, 2023•

edited

Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
Let me fix that.
#88532 (comment)
The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@tannergoodingtannergoodingAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
@neon-sunsetneon-sunsetAug 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
You are correct, my mistake.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄
@stephentoubstephentoub
mentioned this pull request
Aug 14, 2023

Open

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 14, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Oct 26, 2023

Closed

@neon-sunset
neon-sunset
marked this pull request as draft
October 26, 2023 10:52

This was referenced Nov 7, 2023

Closed


Closed


Closed

@ghostghost
closed this
Dec 7, 2023
@ghost
ghost
commented
Dec 7, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 7, 2024

This pull request was closed.

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


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees


No one assigned


Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1


5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh

, '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
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64 - #90527
Closed
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Closed
Fix GetIndexOfFirstNonAsciiByte_Vector not taken on ARM64#90527
neon-sunset wants to merge 4 commits into
dotnet:mainfrom 
neon-sunset:fix-ascii-arm64
Conversation
@neon-sunset
@neon-sunsetneon-sunset
commented
Aug 14, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
@ghostghost
added

area-System.Text.Encoding

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 14, 2023
@ghost
ghost
commented
Aug 14, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-encoding

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #89924
Author:neon-sunset
Assignees:-
Labels:
area-System.Text.Encoding
Milestone:-
stephentoub
stephentoub
reviewed
Aug 14, 2023
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs
 if (!Vector512.IsHardwareAccelerated &&
 !Vector256.IsHardwareAccelerated &&
 (Sse2.IsSupported || AdvSimd.IsSupported))
 if (!Vector512.IsHardwareAccelerated && !Vector256.IsHardwareAccelerated && Sse2.IsSupported)
@stephentoubstephentoubAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
But I'd also explicitly asked about this:
#88532 (comment)

and was told the preferring of the intrinsics path was by design. Maybe it was only measured on x64 and not on Arm?
cc: @tannergooding, @anthonycanino
@neon-sunsetneon-sunsetAug 14, 2023•

edited

Loading

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.

If we do this, then doesn't this change leave a bunch of dead code in GetIndexOfFirstNonAsciiByte_Intrinsified that should be cleaned up? It has multiple blocks dedicated to AdvSimd which would never be reached. At which point the method should also be renamed to be SSE-specific.
Let me fix that.
#88532 (comment)
The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@tannergoodingtannergoodingAug 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The _Vector path is estimated to be ~40% faster on osx-arm64, It is likely the switch was not measured on ARM64.
@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
The PR that introduced the _Vector path (#88532) didn't change _Intrinsified at all (outside using the centralized Vector128.Size constant) and didn't change which Arm64 was calling.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
@neon-sunsetneon-sunsetAug 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@neon-sunset can you clarify please? .NET 7 was using the _Intrinsified path for Arm64: https://github.com/dotnet/runtime/blob/v7.0.10/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs#L81-L91
You are correct, my mistake.
This then wouldn't seem like a regression, but rather simply a case where the new _Vector code is faster for Arm64. If that's the case, could you share your own numbers showing the .NET 7 vs .NET 8 difference (without this PR) and the difference for _Vector vs _Intrinsified for .NET 8 (this PR vs without this PR)?
This is the plan, I'm just waiting for the Release build to finish to post detailed numbers 😄
@stephentoubstephentoub
mentioned this pull request
Aug 14, 2023

Open

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 14, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Oct 26, 2023

Closed

@neon-sunset
neon-sunset
marked this pull request as draft
October 26, 2023 10:52

This was referenced Nov 7, 2023

Closed


Closed


Closed

@ghostghost
closed this
Dec 7, 2023
@ghost
ghost
commented
Dec 7, 2023

Copy link


Copy Markdown

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jan 7, 2024

This pull request was closed.

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


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tannergoodingtannergoodingtannergooding left review comments
Copilot code reviewCopilot
Copilot will automatically review once the pull request is marked ready for review

Assignees


No one assigned


Labels

area-System.Text.Encodingcommunity-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.

GetIndexOfFirstNonAsciiByte_Vector path in Ascii.Utility.cs is never exercised for AdvSimd/SSE4.1


5 participants

@neon-sunset@stephentoub@tannergooding@eiriktsarpalis@tarekgh