Skip to content

Removed JIT (uint) cast workaround to elide bound-checks in CoreLib #67448

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

Merged
jkotas merged 13 commits into from
Jul 12, 2022
Merged

Removed JIT (uint) cast workaround to elide bound-checks in CoreLib #67448

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cf4ccd4
Removed (uint) cast workaround in CoreLib, annotated missing places w…
gfoidl Apr 1, 2022
af9b2d6
Fixed the bug I introduced in TimeSpanParse.TimeSpanTokenizer
gfoidl Apr 1, 2022
19b8e00
Use uint-cast on both sided for clarity
gfoidl Apr 2, 2022
33cd7a1
Added missing namespace (oops)
gfoidl Apr 2, 2022
20842e6
Use uint-division in BitHelper as the JIT can produce more efficient …
gfoidl Apr 4, 2022
da31c0b
Don't have struct local in ValueStringBuilder due to hitting JIT opti…
gfoidl Apr 4, 2022
7e816a5
BitHelper with uint division and without Math.DivRem for even better …
gfoidl Apr 4, 2022
d9f4e01
BitHelper needs local Span due to regression in jit-diff otherwise
gfoidl Apr 4, 2022
034dd89
Fixed dumb copy & paste failure in BitHelper.IsMarked
gfoidl Apr 5, 2022
76d6d2a
Merge branch 'main' into remove-bound-check-hack
gfoidl May 23, 2022
186fb58
Added Debug.Asserts to BitHelper's method that take bitPosition
gfoidl Jul 11, 2022
6afa6c2
BitHelper: comment for workaround to tracking issue
gfoidl Jul 12, 2022
26f459f
Update src/libraries/System.Private.CoreLib/src/System/Collections/Ge…
gfoidl Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
BitHelper: comment for workaround to tracking issue
  • Loading branch information
@gfoidl
gfoidl committed Jul 12, 2022
commit 6afa6c2654cced379d920d97a9e3578f87d3fb6f
4 changes: 4 additions & 0 deletions src/libraries/Common/src/System/Collections/Generic/BitHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ internal void MarkBit(int bitPosition)
Debug.Assert(bitPosition >= 0);

uint bitArrayIndex = (uint)bitPosition / IntSize;

// Workaround for https://github.com/dotnet/runtime/issues/72004
Span<int> span = _span;
if (bitArrayIndex < (uint)span.Length)
{
Expand All @@ -36,6 +38,8 @@ internal bool IsMarked(int bitPosition)
Debug.Assert(bitPosition >= 0);

uint bitArrayIndex = (uint)bitPosition / IntSize;
Copy link
Member

@tannergooding tannergooding Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Same for this one, its doing the same thing but via "more code" now. I'd also expect the JIT was already hoisting the span since its a field of a struct.

Copy link
Member

@jkotas jkotas Jul 11, 2022

Choose a reason for hiding this comment

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

It is not the same thing. Unsigned division is a simple shift. Signed division is more complicated.

Copy link
Member Author

@gfoidl gfoidl Jul 11, 2022

Choose a reason for hiding this comment

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

I'd also expect the JIT was already hoisting the span since its a field of a struct.

Unfortunately that's not the case*. Codegen was validated for this change.
* (at least when the PR was created, at the moment I don't have enough time to check again)

Copy link
Member

@tannergooding tannergooding Jul 11, 2022

Choose a reason for hiding this comment

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

The operation is "the same" even if the codegen is different. If we're doing this specifically to take advantage of the codegen, there should be a comment on it.

More generally, we also need some assert that the input is definitely positive -or- should be taking the input as uint and expecting the caller handle the validation/assertions. The current usages all look to be but its also always the output of GetIndex so without the corresponding outer checks, it'd be pretty easy for someone to accidentally pass in -1 here from some refactoring.

If we're going to be making the method less readable, we should include a comment explaining why these tricks are being used (just as all the current cases have a small // uint cast, per https://github.com/dotnet/runtime/issues/10596 or similar comment). Ideally the issue tracks ways to improve the JIT handling where we can't improve it ourselves via signature changes, locals, or other tweaks.


// Workaround for https://github.com/dotnet/runtime/issues/72004
Span<int> span = _span;
Copy link
Member

@jkotas jkotas Jul 11, 2022

Choose a reason for hiding this comment

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

This one can use a tracking issue. The JIT should be smart enough to make it unnecessary to cache the Span here.

Copy link
Member Author

@gfoidl gfoidl Jul 12, 2022

Choose a reason for hiding this comment

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

Filed #72004 for this.

return
bitArrayIndex < (uint)span.Length &&
Expand Down