Skip to content

Implement alloc::vec::IsZero for Option<$NUM> types #106989

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

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

clubby789
Copy link
Contributor

Fixes #106911

Mirrors the NonZero$NUM implementations with an additional assert_zero_valid.
None::<i32> doesn't stricly satisfy IsZero but for the purpose of allocating we can produce more efficient codegen.

@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@cuviper
Copy link
Member

cuviper commented Jan 17, 2023

None::<i32> doesn't stricly satisfy IsZero but for the purpose of allocating we can produce more efficient codegen.

We should reflect that in the trait docs if we make this change. Rather than "this value's representation is all zeros," we'd now be saying something like "this value is equivalent to a value of all zeros."

@scottmcm
Copy link
Member

Thanks! This looks good to me.

@bors r+ rollup=iffy (it's an optimization, but probably not one that will materially affect compiler perf)


(Though I still hope that one day safe-transmute will let us do this more generally without needing to enumerate all the things here.)

@bors
Copy link
Collaborator

bors commented Jan 18, 2023

📌 Commit 50e9f2e has been approved by scottmcm

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2023
@the8472
Copy link
Member

the8472 commented Jan 18, 2023

We don't really need safe-transmute for this, we need the freeze intrinsic, then we can read all the bytes, not worry about padding and check that they're 0.

@cuviper
Copy link
Member

cuviper commented Jan 18, 2023

(Though I still hope that one day safe-transmute will let us do this more generally without needing to enumerate all the things here.)

What if we had fallible CTFE?

fn is_zero<T: StructuralEq>(x: &T) -> bool {
    match try_const { unsafe { core::mem::MaybeUninit::<T>::zeroed().assume_init() } } {
        Some(zero) => *x == zero,
        None => false,
    }
}

We don't really need safe-transmute for this, we need the freeze intrinsic, then we can read all the bytes, not worry about padding and check that they're 0.

We don't really care if there are non-zero bytes in padding though, only that "significant" bytes are zero. I'm not sure how you would "not worry about padding" in general if we're looking at raw bytes.

@the8472
Copy link
Member

the8472 commented Jan 18, 2023

We don't really care if there are non-zero bytes in padding though, only that "significant" bytes are zero. I'm not sure how you would "not worry about padding" in general if we're looking at raw bytes.

transmute T -> MaybeUninit<[u8]> and then freeze to [u8]. For types without padding this always works, which is a lot more than we currently have! For types with padding it depends on what the freeze op gives us for uninitialized bytes. If it plays nice we'll get zeroes too. If not then it's just a lost optimization.

@the8472
Copy link
Member

the8472 commented Jan 18, 2023

Or maybe the assert_zero_valid intrinsic could be changed to return a bool instead and the asserts could be done in the library?

@cuviper
Copy link
Member

cuviper commented Jan 18, 2023

I think even looking at this PR, you're unlikely to get zeros in a frozen None::<{integer}>.

Or maybe the assert_zero_valid intrinsic could be changed to return a bool

That sounds doable!

@scottmcm
Copy link
Member

I don't think we need full freeze here for the important parts. Freeze might even be suboptimal, since freeze(None::<i32>) == 0_i64 it's allowed to optimize that to false if it wants.

It'd be enough to have a T -> [u8; sizeof(T)] thing that sets any padding inside the type to zero. Or a predicate intrinsic that uses type information to look only at the type's fields instead of looking at raw bytes. (And if it only handled the easy cases that'd be fine, since it could be spec'd as a hint to allow returning conservatively if needed.)

@bors
Copy link
Collaborator

bors commented Jan 19, 2023

⌛ Testing commit 50e9f2e with merge 705a96d...

@bors
Copy link
Collaborator

bors commented Jan 19, 2023

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 705a96d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 19, 2023
@bors bors merged commit 705a96d into rust-lang:master Jan 19, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (705a96d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.1% [0.6%, 1.6%] 2
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
-3.2% [-3.9%, -2.5%] 2
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.8% [-1.8%, -1.8%] 2
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) -1.8% [-1.8%, -1.8%] 2

@clubby789 clubby789 deleted the is-zero-num branch January 19, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vec![None; 1024] randomly gets vectorized or not
7 participants