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

Make `promote_consts` emit the errors when required promotion fails #65946

Merged
merged 4 commits into from Nov 1, 2019

Conversation

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Oct 29, 2019

A very minimal version of #65942.

This will cause a generic "argument X is required to be a constant" message for simd_shuffle LLVM intrinsics instead of the custom one. It may be possible to remove this special-casing altogether after rust-lang/stdarch#825.

r? @eddyb

@@ -360,8 +369,6 @@ impl<'tcx> Validator<'_, 'tcx> {
}
},
Candidate::Argument { bb, index } => {
assert!(self.explicit);

let terminator = self.body[bb].terminator();
match &terminator.kind {
TerminatorKind::Call { args, .. } => {

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 29, 2019

Member

I guess you didn't want to emit the error here?

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Oct 29, 2019

Author Contributor

I can imagine calling validate_candidate and not wanting to emit errors, e.g. for diagnostics.

} else {
if is_shuffle {
span_err!(self.tcx.sess, self.span, E0526,
"shuffle indices are not constant");

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 29, 2019

Member

Probably need to fix tests because this shuffle-specific error message/code isn't emitted anymore? (i.e. now shuffles use the more general error)
I wonder what @gnzlbg thinks (it wouldn't be that hard to keep tbh).

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Oct 29, 2019

Author Contributor

There are no tests for non-const simd_shuffle args AFAICT. I could add one in this PR.

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Oct 29, 2019

Author Contributor

I can store is_shuffle in Candidate::Argument for now. Up to you.

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Oct 29, 2019

Author Contributor

I'm not actually sure how to test simd_shuffle, I might have to add a test in stdarch? The only thing exposed from core are platform-specific intrinsics that wrap the LLVM one and that already have rustc_args_required_const. Let me know if you have ideas.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Oct 30, 2019

Contributor

Probably need to fix tests because this shuffle-specific error message/code isn't emitted anymore? (i.e. now shuffles use the more general error)
I wonder what @gnzlbg thinks (it wouldn't be that hard to keep tbh).

The shuffle intrinsics are only directly used by libcore, so this errors should only be seen by libcore-developers. I don't think having an specific error for this is worth it, the general one should do. rust-lang/stdarch#825 looks like a nice way to solve this problem, so this LGTM.

@ecstatic-morse ecstatic-morse force-pushed the ecstatic-morse:refactor-promotion2 branch from e7b0424 to 5e00a78 Oct 29, 2019
@ecstatic-morse ecstatic-morse force-pushed the ecstatic-morse:refactor-promotion2 branch from 5e00a78 to 122c6fe Oct 29, 2019
@eddyb
eddyb approved these changes Oct 30, 2019
Copy link
Member

eddyb left a comment

r=me after error code cleanup

if !IsNotPromotable::in_operand(self, arg) {
debug!("visit_terminator_kind: candidate={:?}", candidate);
self.promotion_candidates.push(candidate);
} else {
if is_shuffle {
span_err!(self.tcx.sess, self.span, E0526,

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 30, 2019

Member

Please remove (comment out? not sure) E0526 from:

  • src/librustc_mir/error_codes.rs
  • src/tools/tidy/src/error_codes_check.rs

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Oct 30, 2019

Author Contributor

I removed it entirely from tidy but commented it out in librustc_mir where there's other unused errors that are commented out.

It remains as a comment in `error_codes.rs` for consistency with
other unused errors.
@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Oct 30, 2019

All instances of the unused error code have been removed or commented out.

@bors r+ r=eddyb

@bors

This comment was marked as outdated.

Copy link
Contributor

bors commented Oct 30, 2019

📌 Commit 627e3ef has been approved by ecstatic-morse

@bors

This comment was marked as outdated.

Copy link
Contributor

bors commented Oct 30, 2019

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #65941
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 30, 2019

📌 Commit 627e3ef has been approved by eddyb

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Oct 30, 2019

Clearly I need to read up on bors etiquette XD.

tmandry added a commit to tmandry/rust that referenced this pull request Oct 31, 2019
… r=eddyb

Make `promote_consts` emit the errors when required promotion fails

A very minimal version of rust-lang#65942.

This will cause a generic "argument X is required to be a constant" message for `simd_shuffle` LLVM intrinsics instead of the [custom one](https://github.com/rust-lang/rust/blob/caa1f8d7b3b021c86a70ff62d23a07d97acff4c4/src/librustc_mir/transform/qualify_consts.rs#L1616). It may be possible to remove this special-casing altogether after rust-lang/stdarch#825.

r? @eddyb
bors added a commit that referenced this pull request Oct 31, 2019
Rollup of 14 pull requests

Successful merges:

 - #65112 (Add lint and tests for unnecessary parens around types)
 - #65459 (Fix `-Zunpretty=mir-cfg` to render multiple items)
 - #65471 (Add long error explanation for E0578)
 - #65857 (rustdoc: Resolve module-level doc references more locally)
 - #65914 (Use structured suggestion for unnecessary bounds in type aliases)
 - #65945 (Optimize long-linker-command-line test)
 - #65946 (Make `promote_consts` emit the errors when required promotion fails)
 - #65960 (doc: reword iter module example and mention other methods)
 - #65963 (update submodules to rust-lang)
 - #65972 (Fix libunwind build: Define __LITTLE_ENDIAN__ for LE targets)
 - #65977 (Fix incorrect diagnostics for expected type in E0271 with an associated type)
 - #65995 (Add error code E0743 for "C-variadic has been used on a non-foreign function")
 - #65997 (Fix outdated rustdoc of Once::init_locking function)
 - #66005 (vxWorks: remove code related unix socket)

Failed merges:

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Nov 1, 2019
… r=eddyb

Make `promote_consts` emit the errors when required promotion fails

A very minimal version of rust-lang#65942.

This will cause a generic "argument X is required to be a constant" message for `simd_shuffle` LLVM intrinsics instead of the [custom one](https://github.com/rust-lang/rust/blob/caa1f8d7b3b021c86a70ff62d23a07d97acff4c4/src/librustc_mir/transform/qualify_consts.rs#L1616). It may be possible to remove this special-casing altogether after rust-lang/stdarch#825.

r? @eddyb
bors added a commit that referenced this pull request Nov 1, 2019
Rollup of 16 pull requests

Successful merges:

 - #65112 (Add lint and tests for unnecessary parens around types)
 - #65470 (Don't hide ICEs from previous incremental compiles)
 - #65471 (Add long error explanation for E0578)
 - #65857 (rustdoc: Resolve module-level doc references more locally)
 - #65902 (Make ItemContext available for better diagnositcs)
 - #65914 (Use structured suggestion for unnecessary bounds in type aliases)
 - #65946 (Make `promote_consts` emit the errors when required promotion fails)
 - #65960 (doc: reword iter module example and mention other methods)
 - #65963 (update submodules to rust-lang)
 - #65972 (Fix libunwind build: Define __LITTLE_ENDIAN__ for LE targets)
 - #65977 (Fix incorrect diagnostics for expected type in E0271 with an associated type)
 - #65995 (Add error code E0743 for "C-variadic has been used on a non-foreign function")
 - #65997 (Fix outdated rustdoc of Once::init_locking function)
 - #66002 (Stabilize float_to_from_bytes feature)
 - #66005 (vxWorks: remove code related unix socket)
 - #66018 (Revert PR 64324: dylibs export generics again (for now))

Failed merges:

r? @ghost
@bors bors merged commit 627e3ef into rust-lang:master Nov 1, 2019
4 checks passed
4 checks passed
pr Build #20191030.49 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.