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

Use MixedBitSets in const qualif #134438

Merged
merged 2 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 7 additions & 5 deletions compiler/rustc_const_eval/src/check_consts/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use std::fmt;
use std::marker::PhantomData;

use rustc_index::bit_set::BitSet;
use rustc_index::bit_set::MixedBitSet;
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::{
self, BasicBlock, CallReturnPlaces, Local, Location, Statement, StatementKind, TerminatorEdges,
Expand Down Expand Up @@ -246,12 +246,14 @@ where
}

#[derive(Debug, PartialEq, Eq)]
/// The state for the `FlowSensitiveAnalysis` dataflow analysis. This domain is likely homogeneous,
/// and has a big size, so we use a bitset that can be sparse (c.f. issue #134404).
pub(super) struct State {
/// Describes whether a local contains qualif.
pub qualif: BitSet<Local>,
pub qualif: MixedBitSet<Local>,
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth a comment explaining the choice of representation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do that. In general using a BitSet on large domain sizes can create issues like this.

Copy link
Member

Choose a reason for hiding this comment

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

There's no way a contributor could know this. BitSet is the most canonically named type so it's the one developers will reach for by default.

Copy link
Member Author

@lqd lqd Dec 18, 2024

Choose a reason for hiding this comment

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

I don't disagree. Thinking about the domain representation and whether it's dense or sparse is super common in dataflow so "no way to know this" is probably too broad of a brush stroke though.

We could add a comment to BitSet, or the bit_set module, describing and linking to the other bitsets, cc @nnethercote on that.

In this case I've added the requested comment to the analysis state, if you're happy with it, feel free to r you and michael.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be worth renaming BitSet to DenseBitSet perhaps? That way there's not an obvious one to reach for.

But that should be follow-up for sure :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add it to my todo list :)

/// Describes whether a local's address escaped and it might become qualified as a result an
/// indirect mutation.
pub borrow: BitSet<Local>,
pub borrow: MixedBitSet<Local>,
}

impl Clone for State {
Expand Down Expand Up @@ -320,8 +322,8 @@ where

fn bottom_value(&self, body: &mir::Body<'tcx>) -> Self::Domain {
State {
qualif: BitSet::new_empty(body.local_decls.len()),
borrow: BitSet::new_empty(body.local_decls.len()),
qualif: MixedBitSet::new_empty(body.local_decls.len()),
borrow: MixedBitSet::new_empty(body.local_decls.len()),
}
}

Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_index/src/bit_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1191,6 +1191,14 @@ impl<T: Idx> MixedBitSet<T> {
}
}

#[inline]
pub fn clear(&mut self) {
match self {
MixedBitSet::Small(set) => set.clear(),
MixedBitSet::Large(set) => set.clear(),
}
}

bit_relations_inherent_impls! {}
}

Expand Down
Loading