Skip to content

Fix bug in String.Equals unrolling for certain single-char strings - #78190

Merged
EgorBo merged 3 commits into
dotnet:mainfrom
EgorBo:fix-equals-unroll-bug
Nov 11, 2022
Merged

Fix bug in String.Equals unrolling for certain single-char strings#78190
EgorBo merged 3 commits into
dotnet:mainfrom
EgorBo:fix-equals-unroll-bug

Conversation

@EgorBo

@EgorBoEgorBo commented Nov 10, 2022

Copy link
Copy Markdown
Member

Fixes#78169 bug

For single-char strings with "signed" bit (e.g. ((ushort)'鉄') >> 15) we used to do sign-extension instead of zero-extension 😞
From my understanding, only single-char strings (length == 1) of certain two-bytes languages/symbols are affected (looks like mostly around CJK languages). Still, definitely needs to be backported to 7.0

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 10, 2022
@ghostghost assigned EgorBoNov 10, 2022
@ghost

Copy link
Copy Markdown

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

Issue Details

Fixes #78169 bug

For single-char strings with "signed" bit (e.g. ((ushort)'鉄') >> 15) we used to do sign-extension instead of zero-extension 😞
From my understanding, only single-char strings (length == 1) of certain languages are affected. Still, definitely needs to be backported.

Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@EgorBo

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

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

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.

Coding guidelines I think say to always use ASCII, ie use \uNNNN to avoid accidental garbling later. I don't know whether we are actually consistent. But in this case it would perhaps be clearer.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks, changed to \u9244

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks! Let's keep this PR smaller for easier/cleaner backport. @am11 feel free to file a PR to clean that up after

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.

feel free to file a PR to clean that up after

#78265

@EgorBo

Copy link
Copy Markdown
MemberAuthor

@dotnet/jit-contrib @jakobbotsch PTAL, I'm going to backport it once merged

Comment threadsrc/coreclr/jit/importervectorization.cpp Outdated
@JulieLeeMSFTJulieLeeMSFT added this to the 8.0.0 milestone Nov 11, 2022
@EgorBo
EgorBoforce-pushed the fix-equals-unroll-bug branch from 33f4849 to 4e476cfCompareNovember 11, 2022 12:40
Comment threadsrc/coreclr/jit/importervectorization.cpp Outdated
Comment threadsrc/coreclr/jit/importervectorization.cpp Outdated
EgorBoand others added 2 commits November 11, 2022 13:51
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@EgorBo

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3446166446

@github-actions

Copy link
Copy Markdown
Contributor

@EgorBo backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Use cast nodes
Using index info to reconstruct a base tree...
A	src/coreclr/jit/importervectorization.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/importer_vectorization.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/importer_vectorization.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Use cast nodes
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions

Copy link
Copy Markdown
Contributor

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

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

@EgorBo
EgorBo merged commit 0eb7d71 into dotnet:mainNov 11, 2022
@EgorBo

Copy link
Copy Markdown
MemberAuthor

SPMI failures are due to today's JIT-EE guid update

@EgorBo
EgorBo deleted the fix-equals-unroll-bug branch November 11, 2022 16:26
@ghostghost locked as resolved and limited conversation to collaborators Dec 12, 2022
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent behavior in NET7 String.Equals

6 participants

@EgorBo@am11@danmoseley@jakobbotsch@SingleAccretion@JulieLeeMSFT