Don't try to evaluate const blocks during constant promotion #150557
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
S-waiting-on-review
dianne
added
T-lang
|
Since this adds some extra query calls, a perf run may be warranted. @bors try @rust-timer queue This can be mitigated by reordering the logic to only try const-evaluating after we do other checks (e.g. to make sure we can promote the array we're indexing into, for array indexing), but since this fixes a beta regression, I figure I'll start with a small patch in case there's appetite for a backport. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Don't try to evaluate const blocks during constant promotion
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
dianne
force-pushed
the
no-const-block-eval-in-promotion
branch
from
c4492d6 to
304c5af
Compare
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e44c792): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary 2.2%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 477.934s -> 476.786s (-0.24%) |
rustbot
removed
the
S-waiting-on-perf
traviscross
added
I-lang-radar
rustbot
added
the
beta-nominated
| Const::Ty(..) | Const::Val(..) => true, | ||
| Const::Unevaluated(uc, _) => self.tcx.def_kind(uc.def) != DefKind::InlineConst, |
There was a problem hiding this comment.
| Const::Ty(..) | Const::Val(..) => true, | |
| Const::Unevaluated(uc, _) => self.tcx.def_kind(uc.def) != DefKind::InlineConst, | |
| // `Const::Ty` is always a `ConstKind::Param` right now and that can never be turned | |
| // into a mir value for promotion | |
| // FIXME(mgca): do we want uses of type_const to be normalized during promotion? | |
| Const::Ty(..) => false, | |
| Const::Val(..) => true, | |
| // Other kinds of unevaluated's should be able to cause query cycles too, but | |
| // inline consts *always* cause query cycles | |
| Const::Unevaluated(uc, _) => self.tcx.def_kind(uc.def) != DefKind::InlineConst, |
On stable a Const::Ty is always a ConstKind::Param which you can't do anything meaningful with.
Am I correct in assuming non inline consts can also cause a query cycle here? The following example seems to demonstrate this:
const fn foo() -> &'static u32 {
&(10 / <()>::ASSOC)
}
trait Trait {
const ASSOC: u32;
}
impl Trait for () {
const ASSOC: u32 = *foo();
}I think this is the kind of cycle we're talking about here?
There was a problem hiding this comment.
The cycle there has the same shape, but I find it conceptually a bit different, since it arises from a cyclic dependency in user code: by definition, CTFE of <()>::ASSOC requires CTFE of foo, which requires CTFE of <()>::ASSOC. Even if we weren't trying to evaluate <()>::ASSOC during constant promotion, it and foo are both unusable. Since the cycle is unavoidable, I don't think we need to catch it in constant promotion.
The cycle for inline consts is due to how they're borrow-checked along with their typeck root. If borrow-checking the containing body requires evaluating the constant, this gives rise to a cycle, since evaluating the constant requires borrow-checking it, which again means borrow-checking the containing body. However, using the inline const is perfectly fine as long as we don't evaluate it until after borrow-checking.
I'll tack a note onto to the "other kinds of unevaluated's" comment to clarify why the inline const cycle in particular needs to be avoided.
There was a problem hiding this comment.
Added the suggested change with further elaboration in the comment: (diff)
| // At the time #150464 was reported, the index as evaluated before checking whether the indexed | ||
| // expression and index expression could themselves be promoted. This can't be promoted since | ||
| // it references a local, but it still resulted in a query cycle. |
There was a problem hiding this comment.
Does that mean this PR makes that code go from query cycle -> pass? I feel like it'd be nice to be more clear/explicit about which specific code is fixed by this PR. Afaict any code which doesn't actually need promotion to take place but had a const block there will now pass whereas was previously broken by #138499?
There was a problem hiding this comment.
I think that's right, yeah. I've made the wording more explicit and added some additional examples to hopefully make the tests flow better conceptually (and catch if changes to constant promotion would make it become out-of-date): (diff)
This feels not-correct. I would expect that a const block would be "transparent" to static promotion - I would expect that all of these would continue to compile. Changing this to not compiling just to fix an implementation-detail (query cycle) seems wrong. |
|
This is something that'd need a language team decision, but I'd argue long term we shouldn't be implicitly promoting integer division or array accesses at all (per #124328). Having a bunch of special cases for things we can promote makes Rust harder to reason about (and harder to write lints/diagnostics for, IME). In this light, my feeling is that we shouldn't complicate the compiler implementation to make |
|
A more targeted alternative to this PR could be to reorder the logic for checking if array indexing can be used in promoted constants, so that it recursively checks the array and index before evaluating the index to see if it's in-bounds. That'd fix the regression reported in #150464 (since it'd bail when finding that the array can't be used in a promoted constant) but would leave in the query cycles for cases where whether we promote something depends on CTFE of an inline constant (like |
dianne
force-pushed
the
no-const-block-eval-in-promotion
branch
from
304c5af to
d758793
Compare
Also to be clear, there is a clear migration path for affected code: explicit promotion, expressed as |
The |
traviscross
removed
I-lang-nominated
rust-rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
traviscross
added
the
relnotes
| if let TempState::Defined { location: loc, .. } = self.temps[local] | ||
| && let Left(statement) = self.body.stmt_at(loc) | ||
| && let Some((_, Rvalue::Use(Operand::Constant(c)))) = statement.kind.as_assign() | ||
| && self.is_evaluable(c.const_) |
There was a problem hiding this comment.
| && self.is_evaluable(c.const_) | |
| && self.should_evaluate_for_promotion_checks(c.const_) |
maybe. I feel like is_evaluatable is somewhat confusing of a name
There was a problem hiding this comment.
should_evaluate_for_promotion_checks does feel clearer, yeah; I've made that change. It is a bit unfortunate for ease of scanning to go from
Operand::Constant(c) if self.is_evaluable(c.const_) => {to
Operand::Constant(c)
if self.should_evaluate_for_promotion_checks(
c.const_,
) =>
{but the nested/sequential matches that's a part of could maybe benefit from some restructuring anyway, which could remove some of the indentation. I'll save that potential cleanup for another PR
dianne
force-pushed
the
no-const-block-eval-in-promotion
branch
from
d758793 to
ae9b9cb
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
|
||
| struct Thing(i32); | ||
|
|
||
| fn main() { |
There was a problem hiding this comment.
one more thought, this test currently has some statements which should compile and others which should error, can you split them into 2 revisions or tests?
There was a problem hiding this comment.
opted for 2 revisions to keep the context for the tests all in one place
dianne
force-pushed
the
no-const-block-eval-in-promotion
branch
from
ae9b9cb to
20f46a4
Compare
This comment has been minimized.
This comment has been minimized.
dianne
force-pushed
the
no-const-block-eval-in-promotion
branch
from
20f46a4 to
4039cef
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
I'm seeing that failure on other PRs, so it seems like there's a pretty high chance of hitting it at the moment; I'll hold off for now on trying for green CI. In the mean time though, since perf was clean and merge conflicts seem relatively unlikely, @bors rollup |
|
@bors r+ |
rust-bors
bot
added
S-waiting-on-bors
Rollup merge of #150557 - dianne:no-const-block-eval-in-promotion, r=lcnr Don't try to evaluate const blocks during constant promotion As of #138499, trying to evaluate a const block in anything depended on by borrow-checking will result in a query cycle. Since that could happen in constant promotion, this PR adds a check for const blocks there to stop them from being evaluated. Admittedly, this is a hack. See #124328 for discussion of a more principled fix: removing cases like this from constant promotion altogether. To simplify the conditions under which promotion can occur, we probably shouldn't be implicitly promoting division or array indexing at all if possible. That would likely require a FCW and migration period, so I figure we may as well patch up the cycle now and simplify later. Fixes #150464 I'll also lang-nominate this for visibility. I'm not sure there's much to discuss about this PR specifically, but it does represent a change in semantics. In Rust 1.87, the code below compiled. In Rust 1.88, it became a query cycle error. After this PR, it fails to borrow-check because the temporaries can no longer be promoted. ```rust let (x, y, z); // We only promote array indexing if the index is known to be in-bounds. x = &([0][const { 0 }] & 0); // We only promote integer division if the divisor is known not to be zero. y = &(1 / const { 1 }); // Furthermore, if the divisor is `-1`, we only promote if the dividend is // known not to be `int::MIN`. z = &(const { 1 } / -1); // The borrowed temporaries can't be promoted, so they were dropped at the ends // of their respective statements. (x, y, z); ```
…uwer Rollup of 4 pull requests Successful merges: - rust-lang/rust#151161 (std: move time implementations to `sys`) - rust-lang/rust#151694 (more `proc_macro` bridge cleanups) - rust-lang/rust#151711 (stdarch subtree update) - rust-lang/rust#150557 (Don't try to evaluate const blocks during constant promotion)
16 participants
As of #138499, trying to evaluate a const block in anything depended on by borrow-checking will result in a query cycle. Since that could happen in constant promotion, this PR adds a check for const blocks there to stop them from being evaluated.
Admittedly, this is a hack. See #124328 for discussion of a more principled fix: removing cases like this from constant promotion altogether. To simplify the conditions under which promotion can occur, we probably shouldn't be implicitly promoting division or array indexing at all if possible. That would likely require a FCW and migration period, so I figure we may as well patch up the cycle now and simplify later.
Fixes #150464
I'll also lang-nominate this for visibility. I'm not sure there's much to discuss about this PR specifically, but it does represent a change in semantics. In Rust 1.87, the code below compiled. In Rust 1.88, it became a query cycle error. After this PR, it fails to borrow-check because the temporaries can no longer be promoted.