Stream: t-compiler/wg-mir-opt

Topic: Replace fragile erroneous const sys #70820


Santiago Pastorino (Apr 17 2020 at 19:46, on Zulip):

This PR should be ready for review, cc @oli @eddyb

Santiago Pastorino (Apr 17 2020 at 19:47, on Zulip):

one thing I see though is that after running with --bless I see this changes ...

Santiago Pastorino (Apr 17 2020 at 19:47, on Zulip):
diff --git a/src/test/mir-opt/const_allocation2/64bit/rustc.main.ConstProp.after.mir b/src/test/mir-opt/const_allocation2/64bit/rustc.main.ConstProp.after.mir
index e61f0a8b69f..6d2806d820d 100644
--- a/src/test/mir-opt/const_allocation2/64bit/rustc.main.ConstProp.after.mir
+++ b/src/test/mir-opt/const_allocation2/64bit/rustc.main.ConstProp.after.mir
@@ -30,44 +30,44 @@ fn main() -> () {
 }

 alloc0 (static: FOO, size: 16, align: 8) {
-    ╾──────alloc24+0──────╼ 03 00 00 00 00 00 00 00 │ ╾──────╼........
+    ╾──────alloc20+0──────╼ 03 00 00 00 00 00 00 00 │ ╾──────╼........
 }

-alloc24 (size: 72, align: 8) {
-    0x00 │ 00 00 00 00 __ __ __ __ ╾──────alloc9+0───────╼ │ ....░░░░╾──────╼
+alloc20 (size: 72, align: 8) {
+    0x00 │ 00 00 00 00 __ __ __ __ ╾──────alloc4+0───────╼ │ ....░░░░╾──────╼
     0x10 │ 00 00 00 00 00 00 00 00 00 00 00 00 __ __ __ __ │ ............░░░░
-    0x20 │ ╾──────alloc14+0──────╼ 02 00 00 00 00 00 00 00 │ ╾──────╼........
-    0x30 │ 01 00 00 00 2a 00 00 00 ╾──────alloc22+0──────╼ │ ....*...╾──────╼
+    0x20 │ ╾──────alloc9+0───────╼ 02 00 00 00 00 00 00 00 │ ╾──────╼........
+    0x30 │ 01 00 00 00 2a 00 00 00 ╾──────alloc18+0──────╼ │ ....*...╾──────╼
     0x40 │ 03 00 00 00 00 00 00 00                         │ ........
 }

-alloc9 (size: 0, align: 8) {}
+alloc4 (size: 0, align: 8) {}

-alloc14 (size: 16, align: 8) {
-    ╾──────alloc12+0──────╼ ╾──────alloc13+0──────╼ │ ╾──────╼╾──────╼
+alloc9 (size: 16, align: 8) {
+    ╾──────alloc7+0───────╼ ╾──────alloc8+0───────╼ │ ╾──────╼╾──────╼
 }

-alloc12 (size: 1, align: 1) {
+alloc7 (size: 1, align: 1) {
     05                                              │ .
 }

-alloc13 (size: 1, align: 1) {
+alloc8 (size: 1, align: 1) {
     06                                              │ .
 }

-alloc22 (size: 24, align: 8) {
-    0x00 │ ╾──────alloc18+3──────╼ ╾──────alloc19+0──────╼ │ ╾──────╼╾──────╼
-    0x10 │ ╾──────alloc21+2──────╼                         │ ╾──────╼
+alloc18 (size: 24, align: 8) {
+    0x00 │ ╾──────alloc14+3──────╼ ╾──────alloc15+0──────╼ │ ╾──────╼╾──────╼
+    0x10 │ ╾──────alloc17+2──────╼                         │ ╾──────╼
 }

-alloc18 (size: 4, align: 1) {
+alloc14 (size: 4, align: 1) {
     2a 45 15 6f                                     │ *E.o
 }

-alloc19 (size: 1, align: 1) {
+alloc15 (size: 1, align: 1) {
     2a                                              │ *
 }

-alloc21 (size: 4, align: 1) {
+alloc17 (size: 4, align: 1) {
     2a 45 15 6f                                     │ *E.o
 }
diff --git a/src/test/mir-opt/retain-never-const/rustc.no_codegen.PreCodegen.after.mir b/src/test/mir-opt/retain-never-const/rustc.no_codegen.PreCodegen.after.mir
index 2d7a79ee44c..07d28efb9c3 100644
--- a/src/test/mir-opt/retain-never-const/rustc.no_codegen.PreCodegen.after.mir
+++ b/src/test/mir-opt/retain-never-const/rustc.no_codegen.PreCodegen.after.mir
@@ -2,20 +2,10 @@

 fn no_codegen() -> () {
     let mut _0: ();                      // return place in scope 0 at $DIR/retain-never-const.rs:18:20: 18:20
-    let mut _1: !;                       // in scope 0 at $DIR/retain-never-const.rs:19:13: 19:33
     scope 1 {
     }

     bb0: {
-        StorageLive(_1);                 // bb0[0]: scope 0 at $DIR/retain-never-const.rs:19:13: 19:33
-        _1 = const PrintName::<T>::VOID; // bb0[1]: scope 0 at $DIR/retain-never-const.rs:19:13: 19:33
-                                         // ty::Const
-                                         // + ty: !
-                                         // + val: Unevaluated(DefId(0:9 ~ retain_never_const[317d]::{{impl}}[0]::VOID[0]), [T], None)
-                                         // mir::Constant
-                                         // + span: $DIR/retain-never-const.rs:19:13: 19:33
-                                         // + user_ty: UserType(0)
-                                         // + literal: Const { ty: !, val: Unevaluated(DefId(0:9 ~ retain_never_const[317d]::{{impl}}[0]::VOID[0]), [T], None) }
-        unreachable;                     // bb0[2]: scope 0 at $DIR/retain-never-const.rs:19:13: 19:33
+        unreachable;                     // bb0[0]: scope 0 at $DIR/retain-never-const.rs:19:13: 19:33
     }
 }
Santiago Pastorino (Apr 17 2020 at 19:47, on Zulip):

cc @Wesley Wiser also

Santiago Pastorino (Apr 17 2020 at 19:48, on Zulip):

checking if those changes are expected ...

Wesley Wiser (Apr 17 2020 at 19:48, on Zulip):

It looks like a bunch of allocations got removed which is what we wanted to see.

Wesley Wiser (Apr 17 2020 at 19:49, on Zulip):

So good work :thumbs_up: :slight_smile:

Wesley Wiser (Apr 17 2020 at 19:49, on Zulip):

Have you done a perf run by any chance?

Santiago Pastorino (Apr 17 2020 at 19:50, on Zulip):

nope

Wesley Wiser (Apr 17 2020 at 19:50, on Zulip):

I don't think there would be any regressions but we should rollup=never that PR

Santiago Pastorino (Apr 17 2020 at 19:52, on Zulip):

:+1:

Santiago Pastorino (Apr 17 2020 at 19:52, on Zulip):

can I rollup=never before accepting it?

Santiago Pastorino (Apr 17 2020 at 19:52, on Zulip):

maybe we should just comment that for the one that approves this

Wesley Wiser (Apr 17 2020 at 19:53, on Zulip):

Yeah, I think you can do that at any time

Wesley Wiser (Apr 17 2020 at 19:54, on Zulip):

You can check the queue to see if it took effect https://buildbot2.rust-lang.org/homu/queue/rust

Santiago Pastorino (Apr 17 2020 at 20:00, on Zulip):

one pending thing I have is ...

Santiago Pastorino (Apr 17 2020 at 20:00, on Zulip):
Santiago Pastorino (Apr 17 2020 at 20:02, on Zulip):

anyway I think this could be merged as is and that could be done on a separate PR

Santiago Pastorino (Apr 17 2020 at 20:02, on Zulip):

cc @oli

RalfJ (Apr 19 2020 at 08:25, on Zulip):

also I have a terminology question at https://github.com/rust-lang/rust/pull/70820#discussion_r410522129

Santiago Pastorino (Apr 19 2020 at 13:37, on Zulip):

@RalfJ will check this out tomorrow

Santiago Pastorino (Apr 21 2020 at 13:35, on Zulip):

btw, this PR #70820 is ready for review cc @oli @eddyb

oli (Apr 21 2020 at 13:49, on Zulip):

just the the move from visit_body to super_body, then it lgtm

Santiago Pastorino (Apr 21 2020 at 13:50, on Zulip):

@oli right, it makes sense

Santiago Pastorino (Apr 21 2020 at 13:50, on Zulip):

will do it quickly

Santiago Pastorino (Apr 21 2020 at 13:57, on Zulip):

@oli pushed, let's see if CI is happy

Santiago Pastorino (Apr 21 2020 at 14:01, on Zulip):

got an ICE

Santiago Pastorino (Apr 21 2020 at 14:02, on Zulip):
query stack during panic:
#0 [optimized_mir] processing `num::<impl i128>::min_value`  |  = note: this failure-note originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

#1 [const_eval_raw] const-evaluating `num::<impl i128>::min_value`  |  = note: this failure-note originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

#2 [const_eval_raw] const-evaluating `i128::MIN`
#3 [const_eval_validated] const-evaluating + checking `i128::MIN`
#4 [const_eval_validated] const-evaluating + checking `i128::MIN`
#5 [analysis] running analysis passes on this crate
end of query stack
error: could not compile `core`.
Santiago Pastorino (Apr 21 2020 at 14:03, on Zulip):
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/librustc_mir/transform/const_prop.rs:823:38
Santiago Pastorino (Apr 21 2020 at 14:04, on Zulip):

@oli https://github.com/rust-lang/rust/blob/9601d1fc7b448e28675d758e606f8be8f2a9ff7f/src/librustc_mir/transform/const_prop.rs#L823, source_info is None in this case

Santiago Pastorino (Apr 21 2020 at 14:05, on Zulip):

actually, why do we want this in the Visitor? won't this make the code to be evaluated several times? like one for each visitor that doesn't override visit_body or overrides it but calls super_body?

Santiago Pastorino (Apr 21 2020 at 14:07, on Zulip):

I think I've fixed the problem, testing but still ... won't be call this several times?

oli (Apr 21 2020 at 14:10, on Zulip):

sec, let me check the PR diff

oli (Apr 21 2020 at 14:11, on Zulip):

all this does is cause visit_constant to be called, there's no evaluation happening there unless the visitor chooses to do so

Santiago Pastorino (Apr 21 2020 at 14:11, on Zulip):

ohh you're right

Santiago Pastorino (Apr 21 2020 at 14:11, on Zulip):

ok, let's wait for the CI and ship it then :)

oli (Apr 21 2020 at 14:12, on Zulip):

:+1:

oli (Apr 21 2020 at 14:12, on Zulip):

r=me with CI passing

Santiago Pastorino (Apr 21 2020 at 14:30, on Zulip):

got a bunch of failures https://gist.github.com/spastorino/2c3fd74d12ab7a39d073328fbd3b58f3

oli (Apr 21 2020 at 14:33, on Zulip):

hmm... while we could just bless them away, I think for now it would be best to check which visit_constant implementor causes this and make its visit_body function not invoke super_body, but just walk the blocks and nothing else

Santiago Pastorino (Apr 21 2020 at 14:35, on Zulip):

makes sense

Santiago Pastorino (Apr 21 2020 at 15:04, on Zulip):

so as visit_constant implementors we have ExtraComments, ConstPropagator and TypeVerifier

Santiago Pastorino (Apr 21 2020 at 15:07, on Zulip):

gonna try to see which one causes this

oli (Apr 21 2020 at 15:11, on Zulip):

ConstProp is a likely candidate considering the additional diagnostics

Santiago Pastorino (Apr 21 2020 at 15:30, on Zulip):

yeap

Santiago Pastorino (Apr 21 2020 at 15:32, on Zulip):

oli said:

ConstProp is a likely candidate considering the additional diagnostics

I'm not sure what exactly do we want visit_body to do there, the most obvious thing that will work is to not have those calls I've added but keep the rest of the behavior

Last update: Jan 22 2021 at 13:30UTC