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

Function arguments should never get promoted #59724

Merged
merged 4 commits into from
Apr 8, 2019
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
8 changes: 4 additions & 4 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -639,15 +639,15 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
per_local.insert(local);
}
}
cx.per_local[IsNotPromotable].insert(local);
cx.per_local[IsNotConst].insert(local);
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 should do this for all locals where if !temps[local].is_promotable().

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is that we should do this for all locals, not just temps.
That is, don't do it on a case-by-case basis in this match, but rather have this after it:

            if !temps[local].is_promotable() {
                cx.per_local[IsNotConst].insert(local);
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a change that I don't want to backport. are you alright with r+ing this PR and I'll address this immediately in a master-only PR?

Copy link
Member

Choose a reason for hiding this comment

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

There's a risk of there being more issues we haven't hit due to this not being general enough.
This is not just code cleanup, I think right now this is kind of broken.

}

LocalKind::Var if mode == Mode::Fn => {
cx.per_local[IsNotConst].insert(local);
Copy link
Member

Choose a reason for hiding this comment

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

Like, does this mean we'll try to promote a Var in a const fn?!

}

LocalKind::Temp if !temps[local].is_promotable() => {
cx.per_local[IsNotPromotable].insert(local);
cx.per_local[IsNotConst].insert(local);
}

_ => {}
Expand Down Expand Up @@ -817,15 +817,15 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
}
}

// Ensure the `IsNotPromotable` qualification is preserved.
// Ensure the `IsNotConst` qualification is preserved.
// NOTE(eddyb) this is actually unnecessary right now, as
// we never replace the local's qualif, but we might in
// the future, and so it serves to catch changes that unset
// important bits (in which case, asserting `contains` could
// be replaced with calling `insert` to re-set the bit).
if kind == LocalKind::Temp {
Copy link
Member

Choose a reason for hiding this comment

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

Again, here, this seems risky, why don't we do this for all locals?
I think relying on the Var / Temp distinction was a bit of a mistake... I mean, here, as opposed to in the analysis in promote_consts.

if !self.temp_promotion_state[index].is_promotable() {
assert!(self.cx.per_local[IsNotPromotable].contains(index));
assert!(self.cx.per_local[IsNotConst].contains(index));
}
}
}
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/consts/const_arg_local.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// only-x86_64

#[cfg(target_arch = "x86")]
use std::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::*;

unsafe fn pclmul(a: __m128i, b: __m128i) -> __m128i {
let imm8 = 3;
_mm_clmulepi64_si128(a, b, imm8) //~ ERROR argument 3 is required to be a constant
}

fn main() {}
8 changes: 8 additions & 0 deletions src/test/ui/consts/const_arg_local.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: argument 3 is required to be a constant
--> $DIR/const_arg_local.rs:10:5
|
LL | _mm_clmulepi64_si128(a, b, imm8)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

12 changes: 12 additions & 0 deletions src/test/ui/consts/const_arg_promotable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// only-x86_64

#[cfg(target_arch = "x86")]
use std::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::*;

unsafe fn pclmul(a: __m128i, b: __m128i) -> __m128i {
_mm_clmulepi64_si128(a, b, *&mut 42) //~ ERROR argument 3 is required to be a constant
}

fn main() {}
8 changes: 8 additions & 0 deletions src/test/ui/consts/const_arg_promotable.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: argument 3 is required to be a constant
--> $DIR/const_arg_promotable.rs:9:5
|
LL | _mm_clmulepi64_si128(a, b, *&mut 42)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

12 changes: 12 additions & 0 deletions src/test/ui/consts/const_arg_wrapper.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// only-x86_64

#[cfg(target_arch = "x86")]
use std::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::*;

unsafe fn pclmul(a: __m128i, b: __m128i, imm8: i32) -> __m128i {
_mm_clmulepi64_si128(a, b, imm8) //~ ERROR argument 3 is required to be a constant
}

fn main() {}
8 changes: 8 additions & 0 deletions src/test/ui/consts/const_arg_wrapper.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: argument 3 is required to be a constant
--> $DIR/const_arg_wrapper.rs:9:5
|
LL | _mm_clmulepi64_si128(a, b, imm8)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error