Implement const propagation for the MIR `Len` operand #61081

wesleywiser · 2019-05-23T12:35:29Z

rust-highfive · 2019-05-23T13:07:08Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:02e96b48:start=1558614992351566021,finish=1558615085041050302,duration=92689484281
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:28:35]    Compiling rustc-demangle v0.1.10
[00:28:41]    Compiling rustc-std-workspace-alloc v1.0.0 (/checkout/src/tools/rustc-std-workspace-alloc)
[00:28:41]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:28:41]    Compiling hashbrown v0.3.0
[00:28:49] error: internal compiler error: src/librustc_typeck/check/mod.rs:830: can't type-check body of DefId(0:8405 ~ std[a258]::sys[0]::unix[0]::fast_thread_local[0]::register_dtor[0]::[0]::__cxa_thread_atexit_impl[0])
[00:28:49]   --> src/libstd/sys/unix/fast_thread_local.rs:22:9
[00:28:49]    |
[00:28:49] 22 |         static __cxa_thread_atexit_impl: *const libc::c_void;
[00:28:49] 
[00:28:49] thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:572:9
[00:28:49] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:28:49] error: aborting due to previous error
---
[00:28:49] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:28:49] 
[00:28:49] note: rustc 1.36.0-dev running on x86_64-unknown-linux-gnu
[00:28:49] 
[00:28:49] note: compiler flags: -Z external-macro-backtrace -Z unstable-options -Z force-unstable-if-unmarked -C prefer-dynamic -C opt-level=2 -C prefer-dynamic -C debug-assertions=y -C codegen-units=1 -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type dylib --crate-type rlib
[00:28:49] note: some of the compiler flags provided by cargo are hidden
[00:28:49] 
[00:28:49] error: Could not compile `std`.
[00:28:49] 
---
156496 ./src/llvm-project/clang
145240 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu
145236 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release
142484 ./obj/build/bootstrap/debug/incremental/bootstrap-gm2kk8y15os9
142480 ./obj/build/bootstrap/debug/incremental/bootstrap-gm2kk8y15os9/s-fchdycsilv-18hx6zb-1bayru0z1wx58
123648 ./src/llvm-project/llvm/test/CodeGen
108532 ./src/llvm-project/lldb
97584 ./src/llvm-project/clang/test
89964 ./src/llvm-emscripten/test/CodeGen

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

jonas-schievink · 2019-05-23T13:17:57Z

src/librustc_mir/transform/const_prop.rs

+            ) => {
+                // mutable statics can change value at any time so we can't const
+                // propagate their values
+                if self.tcx.is_mutable_static(def_id) {


Does this also need to take internal mutability into account?

It's even worse, the moment you encounter a union or raw pointer you need to give up. Is this change needed for this PR or could it be done in an extra PR?

could it be done in an extra PR?

Possibly? I had some trouble getting a Len operand to appear in the MIR and this was necessary to get the current test case I came up with to pass. The obvious test case

let x= &[1, 2, 3]; x[1]

doesn't seem to generate a Len operand at all.

I'll look into this tonight but this might not be necessary to get this test case to work:

const TEST: &[u32] = &[1, 2, 3]; fn test() -> u32 { TEST[1] }

You get one if you use let x: &[u8] = &[1, 2, 3];. Arrays don't generate Len Rvalues

Oh wow. I didn't realize &[1, 2, 3] got inferred as a &[{integer}; 3] instead of a slice. That makes sense why my original test case didn't work then. A lot of these changes are probably not necessary then.

hmm... The original test wasn't ((&[1, 2, 3]) as &[u32])[1], because I didn't what the MIR after the &[1, 2, 3] to know about the actual array size. But that's not really useful I guess. Feel free to just change it to one big expression and scrap the rest of the test.

Sorry, my previous comment was kind of vague. So far, I haven't been able to find a test case that contains a Len operand that we can const propagate without the changes to handle statics. Here's what I've tried:

let x: &[u32] = &[1, 2, 3]; x[1];

This style test doesn't work at all because x is a local and we don't const propagate locals.

(&[1, 2, 3] as &[u32])[1];

This doesn't compile because it's a non-primitive cast.

static X: &[u32] = &[u32]; X[1];

This could maybe work but would require additional changes like those included in this PR to handle statics.

const X: &[u32] = &[1, 2, 3]; X[1];

This I believe will work once we have support in replace_with_const() for Operand::Indirect.

So, as it stands currently, I don't think there's anyway to write a test which will exercise this part of the const propagator.

The let binding case we can't do until we get some sort of dataflow analysis going

The static case scares me. I'm not sure we should be propagating statics, like ever. A static is something with a name. Reading from it seems like something we shouldn't optimize out, not sure how aggressive we can or want to be there. It's probably alright if the value being read is not from a static mut and the type has no interior mutability, but if we read beyond that (like from things with UnsafeCell or behind raw pointers) we have no guarantees. The problem is, once we read from a static, the propagated value has no notion of "I'm from a static" anymore. So we don't know anymore that we can't just read further values. Most likely not a problem for now, as propagating raw pointer derefs is its very own crazy topic that probably needs UGC discussions. But for now I'd rather not open this box.

The other two cases seem reasonable to me, pick either I guess? Though you may end up needing Indirect support for the cast case, too, so just go with the constant, which requires the least changes.

The static case scares me. I'm not sure we should be propagating statics, like ever.

Yeah, I agree. There's so many edge cases here that it would be very complicated to ensure this was done correctly.

The let binding case we can't do until we get some sort of dataflow analysis going

Do we need full dataflow analysis or is there a simpler thing we could do now and eventually replace with dataflow? For example, I have a change I've been playing with that allows propagating locals which were themselves propagated into. This only is only at the in the ConstPropagator and not the CanConstProp pass so it doesn't affect error reporting. (See e699063 for my code)

hmm... we'd also need to make sure to propagate neither from nor into locals that have more than one mutable usage. You don't want any const prop for let x = if y { 5 } else { 6 }; or for let mut x = 42; if y { x = 9; }

oli-obk · 2019-05-24T12:23:16Z

src/librustc_mir/transform/const_prop.rs

-            // FIXME(oli-obk): evaluate static/constant slice lengths
-            Rvalue::Len(_) => None,
+                let is_slice =
+                    if let ty::Slice(_) = mplace.layout.ty.sty { true }


This is scary. Do we really encounter both &[T] and [T]?

Yeah, after *&[T] is evaluated, we're left with a bare [T].

oli-obk · 2019-05-24T12:23:54Z

src/librustc_mir/transform/const_prop.rs

-            Rvalue::Len(_) => None,
+                let is_slice =
+                    if let ty::Slice(_) = mplace.layout.ty.sty { true }
+                    else { mplace.layout.ty.is_slice() };


this will also return true for *const [T], which we should probably not const prop for now

bors · 2019-05-28T14:55:51Z

☔ The latest upstream changes (presumably #61258) made this pull request unmergeable. Please resolve the merge conflicts.

wesleywiser · 2019-05-28T15:20:06Z

I'm closing this for now since there's a number of issues with the current implementation and I don't see any way to actually test this code right now because of the issues identified here.

Implement const propagation for the MIR Len operand

841c689

rust-highfive assigned oli-obk May 23, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2019

jonas-schievink reviewed May 23, 2019

View reviewed changes

oli-obk reviewed May 24, 2019

View reviewed changes

wesleywiser closed this May 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement const propagation for the MIR `Len` operand #61081

Implement const propagation for the MIR `Len` operand #61081

wesleywiser commented May 23, 2019

rust-highfive commented May 23, 2019

jonas-schievink May 23, 2019

oli-obk May 24, 2019

wesleywiser May 24, 2019

oli-obk May 24, 2019

wesleywiser May 24, 2019

oli-obk May 25, 2019

wesleywiser May 25, 2019

oli-obk May 25, 2019

wesleywiser May 25, 2019

oli-obk May 25, 2019 •

edited

Loading

oli-obk May 24, 2019

wesleywiser May 24, 2019

oli-obk May 24, 2019

bors commented May 28, 2019

wesleywiser commented May 28, 2019

Implement const propagation for the MIR Len operand #61081

Implement const propagation for the MIR Len operand #61081

Conversation

wesleywiser commented May 23, 2019

rust-highfive commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk May 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 28, 2019

wesleywiser commented May 28, 2019

Implement const propagation for the MIR `Len` operand #61081

Implement const propagation for the MIR `Len` operand #61081

oli-obk May 25, 2019 •

edited

Loading