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

promote_consts: some clean-up after experimenting #125853

Merged
merged 5 commits into from
Jun 21, 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: 6 additions & 6 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ where

/// Take an operand, representing a pointer, and dereference it to a place.
/// Corresponds to the `*` operator in Rust.
#[instrument(skip(self), level = "debug")]
#[instrument(skip(self), level = "trace")]
pub fn deref_pointer(
&self,
src: &impl Readable<'tcx, M::Provenance>,
Expand Down Expand Up @@ -533,7 +533,7 @@ where

/// Computes a place. You should only use this if you intend to write into this
/// place; for reading, a more efficient alternative is `eval_place_to_op`.
#[instrument(skip(self), level = "debug")]
#[instrument(skip(self), level = "trace")]
pub fn eval_place(
&self,
mir_place: mir::Place<'tcx>,
Expand Down Expand Up @@ -570,7 +570,7 @@ where

/// Write an immediate to a place
#[inline(always)]
#[instrument(skip(self), level = "debug")]
#[instrument(skip(self), level = "trace")]
pub fn write_immediate(
&mut self,
src: Immediate<M::Provenance>,
Expand Down Expand Up @@ -808,7 +808,7 @@ where
/// Copies the data from an operand to a place.
/// `allow_transmute` indicates whether the layouts may disagree.
#[inline(always)]
#[instrument(skip(self), level = "debug")]
#[instrument(skip(self), level = "trace")]
fn copy_op_inner(
&mut self,
src: &impl Readable<'tcx, M::Provenance>,
Expand Down Expand Up @@ -837,7 +837,7 @@ where
/// `allow_transmute` indicates whether the layouts may disagree.
/// Also, if you use this you are responsible for validating that things get copied at the
/// right type.
#[instrument(skip(self), level = "debug")]
#[instrument(skip(self), level = "trace")]
fn copy_op_no_validate(
&mut self,
src: &impl Readable<'tcx, M::Provenance>,
Expand Down Expand Up @@ -914,7 +914,7 @@ where
/// If the place currently refers to a local that doesn't yet have a matching allocation,
/// create such an allocation.
/// This is essentially `force_to_memplace`.
#[instrument(skip(self), level = "debug")]
#[instrument(skip(self), level = "trace")]
pub fn force_allocation(
&mut self,
place: &PlaceTy<'tcx, M::Provenance>,
Expand Down
32 changes: 15 additions & 17 deletions compiler/rustc_mir_transform/src/promote_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ impl<'tcx> MirPass<'tcx> for PromoteTemps<'tcx> {
let ccx = ConstCx::new(tcx, body);
let (mut temps, all_candidates) = collect_temps_and_candidates(&ccx);

let promotable_candidates = validate_candidates(&ccx, &mut temps, &all_candidates);
let promotable_candidates = validate_candidates(&ccx, &mut temps, all_candidates);

let promoted = promote_candidates(body, tcx, temps, promotable_candidates);
self.promoted_fragments.set(promoted);
Expand Down Expand Up @@ -98,8 +98,8 @@ struct Collector<'a, 'tcx> {
}

impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> {
#[instrument(level = "debug", skip(self))]
fn visit_local(&mut self, index: Local, context: PlaceContext, location: Location) {
debug!("visit_local: index={:?} context={:?} location={:?}", index, context, location);
// We're only interested in temporaries and the return place
match self.ccx.body.local_kind(index) {
LocalKind::Arg => return,
Expand All @@ -111,20 +111,15 @@ impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> {
// then it's constant and thus drop is noop.
// Non-uses are also irrelevant.
if context.is_drop() || !context.is_use() {
debug!(
"visit_local: context.is_drop={:?} context.is_use={:?}",
context.is_drop(),
context.is_use(),
);
debug!(is_drop = context.is_drop(), is_use = context.is_use());
return;
}

let temp = &mut self.temps[index];
debug!("visit_local: temp={:?}", temp);
debug!(?temp);
*temp = match *temp {
TempState::Undefined => match context {
PlaceContext::MutatingUse(MutatingUseContext::Store)
| PlaceContext::MutatingUse(MutatingUseContext::Call) => {
PlaceContext::MutatingUse(MutatingUseContext::Store | MutatingUseContext::Call) => {
TempState::Defined { location, uses: 0, valid: Err(()) }
}
_ => TempState::Unpromotable,
Expand All @@ -137,7 +132,7 @@ impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> {
| PlaceContext::NonMutatingUse(_) => true,
PlaceContext::MutatingUse(_) | PlaceContext::NonUse(_) => false,
};
debug!("visit_local: allowed_use={:?}", allowed_use);
debug!(?allowed_use);
if allowed_use {
*uses += 1;
return;
Expand All @@ -146,6 +141,7 @@ impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> {
}
TempState::Unpromotable | TempState::PromotedOut => TempState::Unpromotable,
};
debug!(?temp);
}

fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) {
Expand Down Expand Up @@ -695,15 +691,12 @@ impl<'tcx> Validator<'_, 'tcx> {
fn validate_candidates(
ccx: &ConstCx<'_, '_>,
temps: &mut IndexSlice<Local, TempState>,
candidates: &[Candidate],
mut candidates: Vec<Candidate>,
) -> Vec<Candidate> {
let mut validator = Validator { ccx, temps, promotion_safe_blocks: None };

candidates.retain(|&candidate| validator.validate_candidate(candidate).is_ok());
candidates
.iter()
.copied()
.filter(|&candidate| validator.validate_candidate(candidate).is_ok())
.collect()
}

struct Promoter<'a, 'tcx> {
Expand Down Expand Up @@ -972,7 +965,12 @@ fn promote_candidates<'tcx>(
candidates: Vec<Candidate>,
) -> IndexVec<Promoted, Body<'tcx>> {
// Visit candidates in reverse, in case they're nested.
debug!("promote_candidates({:?})", candidates);
debug!(promote_candidates = ?candidates);

// eagerly fail fast
if candidates.is_empty() {
return IndexVec::new();
}

let mut promotions = IndexVec::new();

Expand Down
2 changes: 1 addition & 1 deletion library/panic_unwind/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ alloc = { path = "../alloc" }
core = { path = "../core" }
unwind = { path = "../unwind" }
compiler_builtins = "0.1.0"
cfg-if = "1.0"
cfg-if = { version = "1.0", features = ['rustc-dep-of-std'] }
Copy link
Member

Choose a reason for hiding this comment

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

what is this change for?

Copy link
Contributor Author

@tesuji tesuji Jun 1, 2024

Choose a reason for hiding this comment

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

When building with x test test/mir-opt, I noticed that std keeps rebuilding after no changes in it.
Setting CARGO_LOG=cargo::core::compiler::fingerprint=info, it reveals a part of the problem
is that cfg-if built differently for panic_unwind and std.
Having said that, I try again with simple x build libary multiple times without this change,
std is not getting rebuilt when unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to remove this diff if it adds noice.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good find, we should keep this change.


[target.'cfg(not(all(windows, target_env = "msvc")))'.dependencies]
libc = { version = "0.2", default-features = false }
Loading