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.

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

Jump to bottom

Use explicit promotion for constants in repeat expressions #70042

Closed
wants to merge 9 commits into from
Closed

Use explicit promotion for constants in repeat expressions #70042

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
44e8a9a
Fix ui test blessing when a test has an empty stderr file after havin…
oli-obk Mar 16, 2020
85e2c32
Rename `Item` to `ConstCx`.
oli-obk Mar 23, 2020
77345cc
Use ConstCx in more places
oli-obk Apr 2, 2020
ee868e0
Use ConstCx in the promoted collector
oli-obk Apr 2, 2020
29aebf4
Use `ConstCx` for `validate_candidates`
oli-obk Apr 2, 2020
a4e551e
Use explicit promotion for repeat expression elements if the type is …
oli-obk Apr 2, 2020
f7b89da
Catch and fix explicit promotions that fail to actually promote
oli-obk Apr 6, 2020
1530fe0
Rustfmt
oli-obk Apr 7, 2020
d41c8d3
Add test for passing promotion but failing compilation
oli-obk Apr 7, 2020
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
26 changes: 23 additions & 3 deletions src/librustc_mir/transform/promote_consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,26 @@ impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> {
Rvalue::Ref(..) => {
self.candidates.push(Candidate::Ref(location));
}
Rvalue::Repeat(elem, _)
Rvalue::Repeat(elem, n)
if self.ccx.tcx.features().const_in_array_repeat_expressions =>
{
if !elem.ty(&self.ccx.body.local_decls, self.ccx.tcx).is_copy_modulo_regions(
let is_copy = elem.ty(&self.ccx.body.local_decls, self.ccx.tcx).is_copy_modulo_regions(
self.ccx.tcx,
self.ccx.param_env,
self.span,
) {
);
let n = n.try_eval_usize(self.ccx.tcx, self.ccx.param_env);
let length_requires_copy_or_promotion = match n {
// Unevaluable (e.g. too generic) -> assume > 1.
None => true,
Copy link
Member

Choose a reason for hiding this comment

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

So could this lead to a problem if we later figure out that actually, the constant is 1, and then we compile the program after a failed attempt to explicitly promote? (That would trigger the delayed_span_bug now.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the alternative is to fail to compile (!Copy types treat generic lengths as > 1, so if it later happens to be 1 that's irrelevant). While we could permit that, we'd be adding a new source of post monomorphization errors.

Copy link
Member

Choose a reason for hiding this comment

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

I mean I am fine with this as long as it is guaranteed that this cannot ICE due to the safety net delay_span_bug this PR also adds.
I want to be sure that we do not draw ourselves into a corner where, or backwards compatibility reasons, we have to remove that safety net to fix an ICE.

// A single element does not need copying, we can just use the given element.
// Zero elements don't even need that one element.
Some(0..=1) => false,
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// All other lengths require copying (or promotion-"copying" by creating a
// a constant and using that many times).
Some(_) => true,
};
if !is_copy && length_requires_copy_or_promotion {
self.candidates.push(Candidate::Repeat(location));
}
}
Expand Down Expand Up @@ -741,6 +753,14 @@ pub fn validate_candidates(
// and `#[rustc_args_required_const]` arguments here.

let is_promotable = validator.validate_candidate(candidate).is_ok();

if validator.explicit && !is_promotable {
ccx.tcx.sess.delay_span_bug(
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining why this check is absolutely important?
I want to be sure someone doesn't just remove it when it seems convenient.^^

The explicit field's doc comment already says a few things, so here's a proposal:

// If we use explicit validation, we carry the risk of turning a legitimate run-time
// operation into a failing compile-time operation. Make sure that does not happen
// by asserting that there is no possible run-time behavior here in case promotion fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What actually happens is not that we turn it into a failing compile-time op, it's that we don't promote at all, thus keeping the runtime operation, which happened to be the right thing in the cases that the assertion caught.

Copy link
Member

Choose a reason for hiding this comment

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

keeping the runtime operation

We do that because we don't even try. But the branch here is for after we tried, so we cannot be in the case where we want to keep the runtime operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we are, this checks whether we can promote and if not, we remove it from the list. Then we process this list in promote_candidates, which can only fail to promote without ICEing in case of nested promotion candidates, where promoting one candidate eliminated the other candidate because the other candidate is part of the already promoted one.

Copy link
Member

Choose a reason for hiding this comment

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

I am entirely confused now.

Going back a step -- did you want to say above that my proposed comment was incorrect, and suggest a better one? I don't understand what is is you are trying to say here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe your comment is incorrect

// If we use explicit validation, we expect promotion to succeed.
// If an explicit promotion candidate were to get "unpromoted" here,
// that means we keep an operation that we expect to happen at compile-time
// at runtime. In case of repeat elements, this would just cause an error
// later in compilation (due to multiple moves out of a `!Copy` value), but
// for `rustc_arg_required_const` function arguments, where a constant is
// required, ending up with a runtime value would likely ICE the compiler
// during codegen, or silently cause undocumented and undesired LLVM features
// to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RalfJung What do you think about the above comment?

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 we are having different views of the same thing here.

Re: your comment, I am not worried about the case where, if promotion analysis here fails, we ICE the compiler later. I am worried about the case where we do not ICE later. Because that means that, no matter whether promotion succeeds or fails, we produce a program, but the programs may do different things because we use the less conservative, "explicit promotion" rules. That is what I think this comment should explain.

Copy link
Member

Choose a reason for hiding this comment

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

(This is the delay_span_bug that I meant in my other comment, that we should add nevertheless.)

ccx.body.span,
"Explicit promotion requested, but failed to promote",
);
}

match candidate {
Candidate::Argument { bb, index } if !is_promotable => {
let span = ccx.body[bb].terminator().source_info.span;
Expand Down