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.

Sign up for GitHub

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

Jump to bottom

Remove unsafe bool casts #111024

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove unsafe bool casts #111024

wants to merge 4 commits into from

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jan 2, 2025 •
edited
Loading

Contributes to #94941.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 2, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 2, 2025
This was referenced Jan 2, 2025
Open
Open
Open
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 2, 2025

@MihuBot

@MihuBot MihuBot mentioned this pull request Jan 2, 2025
Open
xtqqczze
xtqqczze commented Jan 2, 2025
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Half.cs
@@ -1075,17 +1075,15 @@ public static explicit operator float(Half value)
// Extract exponent bits of value (BiasedExponent is not for here as it performs unnecessary shift)
uint offsetExponent = bitValueInProcess & HalfExponentMask;
// ~0u when value is subnormal, 0 otherwise
uint subnormalMask = (uint)-Unsafe.BitCast<bool, byte>(offsetExponent == 0u);
// ~0u when value is either Infinity or NaN, 0 otherwise
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this comment is incorrect

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 2, 2025 •
edited
Loading

@tannergooding Is this beneficial? 2 bytes higher in code size, but 0.75 lower PerfScore.

MihuBot/runtime-utils#854

 ; Assembly listing for method System.Half:op_Explicit(float):System.Half (FullOpts)
+       xor      ecx, ecx
+       mov      edx, -1
        vucomiss xmm0, xmm0
-       setp     cl
-       movzx    rcx, cl
-       dec      ecx
+       cmovnp   ecx, edx

Related: #61761 (comment)

EgorBo
EgorBo approved these changes Jan 2, 2025
View reviewed changes
Copy link
Member

@EgorBo EgorBo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM otherwise, thanks!. Wonder if we have more such bool <--> int unsafe conversions

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 3, 2025 •
edited
Loading

LGTM otherwise, thanks!. Wonder if we have more such bool <--> int unsafe conversions

These were the only conversions I found searching for:

  • Unsafe.As<bool
  • Unsafe.BitCast<bool
  • *(byte*)&
  • *(sbyte*)&

@EgorBo
Copy link
Member

EgorBo commented Jan 10, 2025

Is this ready?

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 10, 2025 •
edited
Loading

Is this ready?

@EgorBo I had a question about codegen: #111024 (comment)

I'm guessing the diff codegen in that comment is preferable?

@xtqqczze xtqqczze marked this pull request as ready for review January 11, 2025 00:32
@EgorBo EgorBo mentioned this pull request Jan 13, 2025
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EgorBo EgorBo EgorBo approved these changes

Assignees
No one assigned
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xtqqczze @EgorBo