Skip to content

Streamline define_dep_nodes. #152525

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

Open
nnethercote wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −28
Open

Streamline define_dep_nodes. #152525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 4 additions & 27 deletions compiler/rustc_middle/src/dep_graph/dep_node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,56 +14,33 @@ macro_rules! define_dep_nodes {
[$($modifiers:tt)*] fn $variant:ident($($K:tt)*) -> $V:ty,
)*
) => {

#[macro_export]
macro_rules! make_dep_kind_array {
($mod:ident) => {[ $($mod::$variant()),* ]};
}

/// This enum serves as an index into arrays built by `make_dep_kind_array`.
// This enum has more than u8::MAX variants so we need some kind of multi-byte
// encoding. The derived Encodable/Decodable uses leb128 encoding which is
// dense when only considering this enum. But DepKind is encoded in a larger
// struct, and there we can take advantage of the unused bits in the u16.
#[allow(non_camel_case_types)]
#[repr(u16)] // Must be kept in sync with the inner type of `DepKind`.
enum DepKindDefs {
$( $( #[$attr] )* $variant),*
}

#[allow(non_upper_case_globals)]
pub mod dep_kinds {
use super::*;

$(
// The `as u16` cast must be kept in sync with the inner type of `DepKind`.
pub const $variant: DepKind = DepKind::new(DepKindDefs::$variant as u16);
pub const $variant: DepKind = DepKind::new(${index()});
)*
}

// This checks that the discriminants of the variants have been assigned consecutively
// from 0 so that they can be used as a dense index.
pub(crate) const DEP_KIND_VARIANTS: u16 = {
let deps = &[$(dep_kinds::$variant,)*];
let mut i = 0;
while i < deps.len() {
if i != deps[i].as_usize() {
panic!();
}
i += 1;
}
deps.len() as u16
};
Comment on lines -46 to -56
Copy link
Member

@Zalathar Zalathar Feb 12, 2026

Choose a reason for hiding this comment

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

Do we have any other check that the numeric values continue to be dense and ascending from 0?

That's currently still true in the new implementation, but I don't know if there's anything ensuring that it remains true.

Copy link
Contributor Author

@nnethercote nnethercote Feb 12, 2026

Choose a reason for hiding this comment

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

The numeric values come from the macro metavar ${index()}, which is "The current index of the inner-most repetition.". I.e. it's just a 0..n counter.

AFAICT this sanity check was overkill for the enum case, because enums are (I think?) guaranteed to be numbered from 0..n but I can see why someone might be uncertain about that and want to check it. But checking that ${index()} actually implements 0..n seems paranoid, that's fully in "I don't even trust the compiler" territory.

Copy link
Member

@saethlin saethlin Feb 13, 2026

Choose a reason for hiding this comment

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

This check seems awfully familiar, as if I wrote it. If I did, I was unsure at the time if it was justified, and the cost seemed acceptable at the time. Fine with me if it's now too clunky.

Copy link
Member

@Zalathar Zalathar Feb 14, 2026

Choose a reason for hiding this comment

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

My concern is not whether ${index()} implements 0..n, but rather that if someone tries to modify this code without knowing that the 0..n is load-bearing, then they'll end up with either mysterious crashes when running the compiler, or (worst-case) very subtle incremental-compilation bugs.

Copy link
Member

@Zalathar Zalathar Feb 14, 2026

Choose a reason for hiding this comment

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

All that said, after #152516 has landed I question whether DepKind needs to be a u16 newtype at all.

My understanding is that #115920 split off DepKind from the enum, because the enum was in rustc_middle but DepKind wanted to be upstream in rustc_query_system.

If DepKind is moved back to rustc_middle, we should be able to just make it an enum again.

Copy link
Member

@Zalathar Zalathar Feb 14, 2026

Choose a reason for hiding this comment

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

This check seems awfully familiar, as if I wrote it.

pub(crate) const NUM_DEP_KIND_VARIANTS: u16 = ${count($variant)};

/// List containing the name of each dep kind as a static string,
/// indexable by `DepKind`.
pub(crate) const DEP_KIND_NAMES: &[&str] = &[
$( self::label_strs::$variant, )*
$( stringify!($variant), )*
];

pub(super) fn dep_kind_from_label_string(label: &str) -> Result<DepKind, ()> {
match label {
$( self::label_strs::$variant => Ok(self::dep_kinds::$variant), )*
$( stringify!($variant) => Ok(self::dep_kinds::$variant), )*
Copy link
Contributor

@petrochenkov petrochenkov Feb 12, 2026

Choose a reason for hiding this comment

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

This actually reverts a recent change from @Zalathar (17c5b7a).
I guess there are different opinions on what is more streamlined (I'm not sure personally).

Copy link
Member

@Zalathar Zalathar Feb 12, 2026

Choose a reason for hiding this comment

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

At the time I was thinking that it would be good to reduce the amount of stringify!($name), but I'm not actually very confident about that being the right direction.

So with hindsight, I don't really object to changing it back.

Copy link
Contributor Author

@nnethercote nnethercote Feb 12, 2026

Choose a reason for hiding this comment

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

For me, mentally expanding stringify!($variant) is easy, while working out what self::label_strs::$variant means is harder because I have to look further down.

_ => Err(()),
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/dep_graph/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ impl Deps for DepsType {
const DEP_KIND_RED: DepKind = dep_kinds::Red;
const DEP_KIND_SIDE_EFFECT: DepKind = dep_kinds::SideEffect;
const DEP_KIND_ANON_ZERO_DEPS: DepKind = dep_kinds::AnonZeroDeps;
const DEP_KIND_MAX: u16 = dep_node::DEP_KIND_VARIANTS - 1;
const DEP_KIND_MAX: u16 = dep_node::NUM_DEP_KIND_VARIANTS - 1;
}

impl<'tcx> DepContext for TyCtxt<'tcx> {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#![feature(gen_blocks)]
#![feature(if_let_guard)]
#![feature(intra_doc_pointers)]
#![feature(macro_metavar_expr)]
#![feature(min_specialization)]
#![feature(negative_impls)]
#![feature(never_type)]
Expand Down
Loading