Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat TZCNT/POPCNT/LZCNT as never negative #64951

Merged
merged 8 commits into from
Feb 21, 2022

Conversation

trympet
Copy link
Contributor

@trympet trympet commented Feb 8, 2022

Check for population count, trailing zero count, and leading zero count intrinsics when determining integral rage.

This change makes most of these casts redundant:
image

Closes #64909

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 8, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed community-contribution Indicates that the PR has been added by a community member labels Feb 8, 2022
@ghost
Copy link

ghost commented Feb 8, 2022

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

Issue Details

Check for population count, trailing zero count, and leading zero count intrinsics when determining integral rage.

Closes #64909

Author: trympet
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@dnfadmin
Copy link

dnfadmin commented Feb 8, 2022

CLA assistant check
All CLA requirements met.

@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Feb 8, 2022
@EgorBo
Copy link
Member

EgorBo commented Feb 8, 2022

Looks like this PR found a couple of improvements 👍

Top method improvements (bytes):
          -3 (-0.11 % of base) : 18228.dasm - HttpParser`1:ParseHeaders(Http1ParsingHandler,byref):bool:this
          -3 (-0.11 % of base) : 19599.dasm - HttpParser`1:ParseHeaders(Http1ParsingHandler,byref):bool:this
          -3 (-0.11 % of base) : 46626.dasm - HttpParser`1:ParseHeaders(Http1ParsingHandler,byref):bool:this
          -3 (-0.12 % of base) : 49052.dasm - HttpParser`1:ParseHeaders(Http1ParsingHandler,byref):bool:this
          -3 (-0.12 % of base) : 52210.dasm - HttpParser`1:ParseHeaders(Http1ParsingHandler,byref):bool:this
          -3 (-0.11 % of base) : 55814.dasm - HttpParser`1:ParseHeaders(Http1ParsingHandler,byref):bool:this
          -3 (-0.14 % of base) : 10768.dasm - HttpParser`1:ParseHeaders(ParsingAdapter,byref):bool:this
          -3 (-0.14 % of base) : 11823.dasm - HttpParser`1:ParseHeaders(ParsingAdapter,byref):bool:this
          -3 (-0.14 % of base) : 12311.dasm - HttpParser`1:ParseHeaders(ParsingAdapter,byref):bool:this
          -3 (-0.08 % of base) : 49311.dasm - HttpParser`1:ParseMultiSpanHeader(Http1ParsingHandler,byref):int:this
          -3 (-0.08 % of base) : 52524.dasm - HttpParser`1:ParseMultiSpanHeader(Http1ParsingHandler,byref):int:this
          -3 (-0.27 % of base) : 39745.dasm - Utf8Utility:GetPointerToFirstInvalidByte(long,int,byref,byref):long
          -3 (-0.19 % of base) : 44609.dasm - Utf8Utility:GetPointerToFirstInvalidByte(long,int,byref,byref):long
          -3 (-0.20 % of base) : 50722.dasm - Utf8Utility:GetPointerToFirstInvalidByte(long,int,byref,byref):long
          -3 (-0.19 % of base) : 63703.dasm - Utf8Utility:GetPointerToFirstInvalidByte(long,int,byref,byref):long
          -2 (-0.15 % of base) : 49316.dasm - BufferExtensions:PositionOfAny(byref,ubyte,ubyte):Nullable`1
          -2 (-0.15 % of base) : 52527.dasm - BufferExtensions:PositionOfAny(byref,ubyte,ubyte):Nullable`1
          -2 (-0.29 % of base) : 10762.dasm - MemoryExtensions:IndexOfAny(ReadOnlySpan`1,ubyte,ubyte):int
          -2 (-0.29 % of base) : 11809.dasm - MemoryExtensions:IndexOfAny(ReadOnlySpan`1,ubyte,ubyte):int
          -2 (-0.29 % of base) : 18229.dasm - MemoryExtensions:IndexOfAny(ReadOnlySpan`1,ubyte,ubyte):int

these functionf are pretty perf sensitive + we won't have to uglify code with uint casts, you can probably even remove some existing uint casts (e.g. I put it in IndexOf) if you want 🙂

@trympet
Copy link
Contributor Author

trympet commented Feb 8, 2022

these functionf are pretty perf sensitive + we won't have to uglify code with uint casts, you can probably even remove some existing uint casts (e.g. I put it in IndexOf) if you want 🙂

@EgorBo, Are you referring to this, for instance?

// Find bitflag offset of first match and add to current offset
return (int)(offset + (uint)BitOperations.TrailingZeroCount(matches));

@EgorBo
Copy link
Member

EgorBo commented Feb 9, 2022

these functionf are pretty perf sensitive + we won't have to uglify code with uint casts, you can probably even remove some existing uint casts (e.g. I put it in IndexOf) if you want 🙂

@EgorBo, Are you referring to this, for instance?

// Find bitflag offset of first match and add to current offset
return (int)(offset + (uint)BitOperations.TrailingZeroCount(matches));

no, I meant this: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs#L612

@trympet
Copy link
Contributor Author

trympet commented Feb 9, 2022

these functionf are pretty perf sensitive + we won't have to uglify code with uint casts, you can probably even remove some existing uint casts (e.g. I put it in IndexOf) if you want 🙂

@EgorBo, Are you referring to this, for instance?

// Find bitflag offset of first match and add to current offset
return (int)(offset + (uint)BitOperations.TrailingZeroCount(matches));

no, I meant this: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs#L612

I'd defer it to a new PR, due to lack of time on my part.

@EgorBo
Copy link
Member

EgorBo commented Feb 13, 2022

@trympet thanks!

@JulieLeeMSFT
Copy link
Member

Thanks @trympet for your first contribution to JIT.

@EgorBo EgorBo merged commit dc99791 into dotnet:main Feb 21, 2022
@EgorBo
Copy link
Member

EgorBo commented Feb 21, 2022

I didn't realize I forgot to merge :-)

@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR 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.

Treat TZCNT/POPCNT/LZCNT as never negative
6 participants