Evaluate fixed-length array length expressions lazily. #44275

eddyb · 2017-09-02T16:46:48Z

This is in preparation for polymorphic array lengths (aka [T; T::A]) and const generics.
We need deferred const-evaluation to break cycles when array types show up in positions which require knowing the array type to typeck the array length, e.g. the array type is in a where clause.

The final step - actually passing bounds in scope to array length expressions from the parent - is not done because it still produces cycles when normalizing ParamEnvs, and @nikomatsakis' in-progress lazy normalization work is needed to deal with that uniformly.

However, the changes here are still useful to unlock work on const generics, which @EpicatSupercell manifested interest in, and I might be mentoring them for that, but we need this baseline first.

r? @nikomatsakis cc @oli-obk

eddyb · 2017-09-02T16:49:56Z

Started a couple crater runs for this PR and my hacky branch enabling [T; T::A].

kennytm · 2017-09-02T17:38:37Z

Failed to link rustc-main on Travis CI (stage1-rustc).

[00:33:19]   = note: /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_trans-c92f3e9b5e9f9868.so: undefined reference to `_$LT$$u5b$u8$u3b$$u20$Const$u20$$u7b$$u20$ty.$u20$usize$C$$u20$val.$u20$Unevaluated$LP$DefId$u20$$u7b$$u20$krate.$u20$CrateNum$LP$30$RP$$C$$u20$node.$u20$DefIndex$LP$2147484848$RP$$u20$$u3d$$GT$$u20$rustc_data_structures$u2f$a34856b..stable_hasher$u5b$0$u5d$..$u7b$$u7b$impl$u7d$$u7d$$u5b$2$u5d$..$u7b$$u7b$initializer$u7d$$u7d$$u5b$0$u5d$$u20$$u7d$$C$$u20$Slice$LP$$u5b$$u5d$$RP$$RP$$u20$$u7d$$u5d$$u20$as$u20$rustc_data_structures..stable_hasher..StableHasherResult$GT$::finish::h51cdc1bd689ed229'

That thing demangled is:

<[u8; Const { 
    ty. usize, 
    val. Unevaluated(DefId { 
        krate. CrateNum(30), 
        node. DefIndex(2147484848) => rustc_data_structures/a34856b..stable_hasher[0]..{{impl}}[2]..{{initializer}}[0] 
    }, Slice([])) 
}] as rustc_data_structures..stable_hasher..StableHasherResult>::finish::h51cdc1bd689ed229

eddyb · 2017-09-02T18:02:24Z

That's funny - I guess I never tried to bootstrap!

kennytm · 2017-09-02T18:18:46Z

If I understand correctly the symbol is referring to this impl in librustc_data_structures/stable_hasher.rs. The constant 20 is unevaluated. Maybe due to the CrateNum or the hash "a34856b" the symbol is not found from rustc_trans.

impl StableHasherResult for [u8; 20] {
    fn finish(mut hasher: StableHasher<Self>) -> Self {
        ...
    }
}

eddyb · 2017-09-02T18:20:29Z

@kennytm Yeah, I just missed a spot - technically that location would already have an issue with associated types but maybe that one is harder to exhibit.

EDIT: nevermind, this is not a normalization issue, just a printing one. Fix underway.

eddyb · 2017-09-02T19:25:52Z

@nikomatsakis This is horrible, impl Trait for [T; expr] requiring printing [T; expr] when printing the path of expr so I can't just print the path.

eddyb · 2017-09-02T20:55:41Z

I ended up going with a simpler but less informative output for now to side-step the issue.

eddyb · 2017-09-03T01:00:10Z

Crater report shows no regressions, so this is probably fine for now.

Mark-Simulacrum

Feel free to ignore these comments.

Mark-Simulacrum · 2017-09-03T01:06:12Z

src/librustc/middle/const_val.rs

-        }
+#[derive(Copy, Clone, Debug, Hash, RustcEncodable, Eq, PartialEq)]
+pub struct ByteArray<'tcx> {
+    pub data: &'tcx [u8],


Making this a Slice would seem like a good idea (and other similar cases below) but I guess it can be left for later; I suppose doing that now would possibly require more guarantees on the part of the code?

Mark-Simulacrum · 2017-09-03T01:13:43Z

src/librustc/ty/structural_impls.rs

@@ -58,6 +59,13 @@ impl<'tcx, T: Lift<'tcx>, E: Lift<'tcx>> Lift<'tcx> for Result<T, E> {
    }
 }

+impl<'tcx, T: Lift<'tcx>> Lift<'tcx> for Box<T> {


This feels suspicious. Why do we have boxes of pointers? Maybe I'm not understanding something...

Mark-Simulacrum · 2017-09-03T01:16:29Z

src/librustc/ty/structural_impls.rs

+            ConstVal::Aggregate(ConstAggregate::Struct(fields)) => {
+                let new_fields: Vec<_> = fields.iter().map(|&(name, v)| {
+                    (name, v.fold_with(folder))
+                }).collect();


This looks terrible for performance. Could we use an AccumulateVec here (and in the below code)? Ideally, of course, we wouldn't need to collect at all when the vectors are equal, but other than adding a boolean to keep track of that as we go there's probably little we can do.

Aggregate should be rare and unused most of the time - it will be soon replaced by a miri allocation instead.

Mark-Simulacrum · 2017-09-03T01:19:30Z

src/librustc_const_eval/eval.rs

        }
      }
      hir::ExprType(ref e, _) => cx.eval(e)?,
      hir::ExprTup(ref fields) => {
-        Tuple(fields.iter().map(|e| cx.eval(e)).collect::<Result<_, _>>()?)
+        let values = fields.iter().map(|e| cx.eval(e)).collect::<Result<Vec<_>, _>>()?;
+        mk_const(Aggregate(Tuple(tcx.alloc_const_slice(&values))))


Shouldn't we be using mk_const_slice here? That would avoid the Vec in the common(?) case of having less than eight fields.

There seem to be more cases of this in this file as well.

Mark-Simulacrum · 2017-09-03T01:22:11Z

src/librustc_metadata/decoder.rs

+impl<'a, 'tcx> SpecializedDecoder<ByteArray<'tcx>> for DecodeContext<'a, 'tcx> {
+    fn specialized_decode(&mut self) -> Result<ByteArray<'tcx>, Self::Error> {
+        Ok(ByteArray {
+            data: self.tcx().alloc_byte_array(&Vec::decode(self)?)


Do we not have a decoder for AcccumulateVec yet? We should use that I think here if possible...

bors · 2017-09-03T15:24:19Z

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

oli-obk · 2017-09-04T13:33:55Z

src/test/compile-fail/const-fn-error.rs

@@ -23,5 +23,5 @@ const fn f(x: usize) -> usize {

 #[allow(unused_variables)]
 fn main() {
-    let a : [i32; f(X)]; //~ NOTE for array length here
+    let a : [i32; f(X)]; //~ NOTE for constant expression here


This is a slight decrease in diagnostic quality

With moving to const generics the harcoded messages seem bizarre at best to me - the expression happens to be in an array length, the span is more important that that.
The description could potentially be recovered by walking up the HIR but that only works in the local crate.

oli-obk · 2017-09-04T13:34:58Z

src/test/compile-fail/const-size_of-cycle.rs

@@ -8,11 +8,12 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

+// error-pattern: unsupported cyclic reference between types/traits detected


Change it into a ui test?

Oh, I just realized this is a bit suboptimal. Or rather, the span is lost of some reason.

~~Oh this is a bug with ParamEnvAnd Key impl for the query system!~~

Welp, no, this is the best I get right now:

error[E0391]: unsupported cyclic reference between types/traits detected --> /home/eddy/Projects/rust-2/src/libcore/mem.rs:192:14 | 192 | unsafe { intrinsics::size_of::<T>() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^ cyclic reference |

There is a note at the bottom of the cycle which shows the expression that triggered the cycle, so not all is lost!

However, I believe there are two ParamEnv's with different Reveal modes on the cycle stack, so the cycle isn't detected early enough... cc @nikomatsakis

oli-obk · 2017-09-04T13:35:38Z

src/librustc_trans/mir/constant.rs

@@ -962,16 +960,17 @@ impl<'a, 'tcx> MirContext<'a, 'tcx> {
        debug!("trans_constant({:?})", constant);
        let ty = self.monomorphize(&constant.ty);
        let result = match constant.literal.clone() {
-            mir::Literal::Item { def_id, substs } => {


eddyb · 2017-09-06T02:21:54Z

@bors try

bors · 2017-09-06T02:22:05Z

⌛ Trying commit e873b4f63ea3f93174bd68cc416701f7f276aa35 with merge 2a8d01143216b1b287134b3f2026fa1b871117d9...

eddyb · 2017-09-06T02:22:49Z

@Mark-Simulacrum ^^ let's see if we can get some perf comparisons from this!
I'm curious because it defers evaluation of and later normalizes array lengths like associated types.

bors · 2017-09-06T05:06:39Z

☀️ Test successful - status-travis
State: approved= try=True

Mark-Simulacrum · 2017-09-06T13:05:25Z

Looks like a regression on most benchmarks: http://perf.rust-lang.org/compare.html?commit_a=2f1ef9ef1181298d46e79d5dde6bafeb6483926f&commit_b=2a8d01143216b1b287134b3f2026fa1b871117d9&stat=instructions%3Au.

eddyb · 2017-09-06T13:37:51Z

@Mark-Simulacrum Hmm, regression is expected of course, but I can't tell how bad that is just from looking at it - is there any per-pass timing?

Mark-Simulacrum · 2017-09-06T15:19:43Z

No, per-pass timing isn't currently available. It seems to be in higher demand than I would expect, so I've filed rust-lang/rustc-perf#147 for discussion.

Mark-Simulacrum · 2017-09-06T15:21:53Z

It may also be helpful to look at the timing data (though not sure how reliable I'd consider it) for the diff: http://perf.rust-lang.org/compare.html?commit_a=2f1ef9ef1181298d46e79d5dde6bafeb6483926f&commit_b=2a8d01143216b1b287134b3f2026fa1b871117d9&stat=cpu-clock. Not sure if you've already examined that though.

nikomatsakis

On the whole, this all makes a lot of sense. I left some questions though.

nikomatsakis · 2017-09-07T14:43:02Z

src/librustc/infer/mod.rs

@@ -493,7 +494,7 @@ impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {
        let param_env = ty::ParamEnv::empty(Reveal::All);
        let value = self.erase_regions(value);

-        if !value.has_projection_types() {
+        if !value.has_projections() {


Idle speculation: I wonder if we should call this requires_normalization, at some point.

needs_normalizing?

nikomatsakis · 2017-09-07T14:49:47Z

src/librustc/traits/fulfill.rs

@@ -540,6 +540,29 @@ fn process_predicate<'a, 'gcx, 'tcx>(
                }
            }
        }
+
+        ty::Predicate::ConstEvaluatable(def_id, substs) => {
+            match selcx.tcx().lift_to_global(&obligation.param_env) {


Hmm, this part seems...a bit suspicious somehow. For now it's probably ok to just pass None results through, but I think we're going to want to revisit what's happening here. I guess that -- in the end -- my plan to "chalkify" things will mean that all param-env and queries can be converted into a closed (and global) form, so this will be ok.

nikomatsakis · 2017-09-07T14:58:10Z

src/librustc/traits/project.rs

+
+    fn fold_const(&mut self, constant: &'tcx ty::Const<'tcx>) -> &'tcx ty::Const<'tcx> {
+        if let ConstVal::Unevaluated(def_id, substs) = constant.val {
+            if substs.needs_infer() {


wait -- what is happening here -- can you leave a comment? I think what is happening is that, if inference results are needed, we are taking a kind of "stab" at doing the evaluation with "generic" substitutions, just to see if it cares at all about the results of inference, is that right?

Yes, should I try freshening for this instead?

Hmm. Well, freshening will substitute FreshTy values, which most of the system is not setup to cope with. I'm not sure I'd go down that road (yet). I do think as part of reworking traits -- and especially implementing higher-ranked trait bounds on types -- we would have the option of substituting skolemized things ...

... I guess plausibly you could freshen, create a fresh inference context, remap those freshened types to fresh variables in there ... this is sort of roughly what chalk does ... but would the const-eval code maybe unify them in any way, or would it just fail to make progress if it encountered an unresolved type variable?

const-eval is global so it'd just error if it can't make a decision.

nikomatsakis · 2017-09-07T14:58:47Z

src/librustc/traits/select.rs

@@ -687,6 +687,21 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
                    }
                }
            }
+
+            ty::Predicate::ConstEvaluatable(def_id, substs) => {
+                match self.tcx().lift_to_global(&(obligation.param_env, substs)) {


Not entirely clear to me why this would behave differently from the code above, which seems to have some more "fallback" options...

They should just both use freshening, right?

I think they should do the same, in any case

nikomatsakis · 2017-09-07T16:54:20Z

src/librustc/ty/relate.rs

+                            expected_found(relation, &sz_a_u64, &sz_b_u64)))
+                    }
+                }
+                _ => Ok(tcx.types.err)


So, it's ok to use err here because we currently expect these to always be evaluatable? Can we add a comment about it? Also, I'd prefer the return type of the closure to be Result<u64, ErrorRepored>, rather than Option<u64>, since it makes it clearer in the type what is expected, and we are less likely to make the closure return None without thinking about it (I personally would also write (Err(ErrorReported), _) | (_, Err(ErrorReported)) => { instead of _, as insurance against the type changing...)

I have a delay_span_bug so it's impossible to produce tcx.types.err without a compiler error. We need lazy normalization to really do anything smarter here.

bors · 2017-09-08T14:47:24Z

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

bors · 2017-09-11T01:06:14Z

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

nikomatsakis · 2017-09-11T21:21:41Z

@bors r+

OK, so, as per our discussion on IRC, there are various things we can do better here, but what you're doing now -- particularly around the freshening bits -- seems ok (not unsound, anyway). I feel like anything we do going forward should be compatible with it.

bors · 2017-09-11T21:21:42Z

📌 Commit 57ebd28 has been approved by nikomatsakis

bors · 2017-09-12T04:14:13Z

⌛ Testing commit 57ebd28 with merge 3cb24bd...

@nikomatsakis

Evaluate fixed-length array length expressions lazily. This is in preparation for polymorphic array lengths (aka `[T; T::A]`) and const generics. We need deferred const-evaluation to break cycles when array types show up in positions which require knowing the array type to typeck the array length, e.g. the array type is in a `where` clause. The final step - actually passing bounds in scope to array length expressions from the parent - is not done because it still produces cycles when *normalizing* `ParamEnv`s, and @nikomatsakis' in-progress lazy normalization work is needed to deal with that uniformly. However, the changes here are still useful to unlock work on const generics, which @EpicatSupercell manifested interest in, and I might be mentoring them for that, but we need this baseline first. r? @nikomatsakis cc @oli-obk

bors · 2017-09-12T06:49:40Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 3cb24bd to master...

rust-highfive assigned nikomatsakis Sep 2, 2017

eddyb force-pushed the deferred-ctfe branch 2 times, most recently from 49e8ea5 to 3a15be7 Compare September 2, 2017 19:12

eddyb force-pushed the deferred-ctfe branch from 3a15be7 to acd89e7 Compare September 2, 2017 20:55

Mark-Simulacrum reviewed Sep 3, 2017

View reviewed changes

eddyb force-pushed the deferred-ctfe branch from acd89e7 to 2df7685 Compare September 3, 2017 16:22

oli-obk requested changes Sep 4, 2017

View reviewed changes

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2017

nikomatsakis approved these changes Sep 7, 2017

View reviewed changes

eddyb force-pushed the deferred-ctfe branch from e873b4f to 51edea7 Compare September 8, 2017 00:37

eddyb force-pushed the deferred-ctfe branch from 51edea7 to 0365c26 Compare September 8, 2017 19:02

eddyb added 7 commits September 11, 2017 08:41

rustc: intern ConstVal's in TyCtxt.

50076b0

rustc: introduce ty::Const { ConstVal, Ty }.

932289c

rustc: replace usize with u64 and ConstUsize.

3ce31eb

rustc: use ty::Const for the length of TyArray.

8a9b78f

rustc: remove obsolete const_val::ErrKind::{Negate,Not}On.

8821761

rustc: evaluate fixed-length array length expressions lazily.

74349fa

rustc: use ConstVal::Unevaluated instead of mir::Literal::Item.

57ebd28

eddyb force-pushed the deferred-ctfe branch from 0365c26 to 57ebd28 Compare September 11, 2017 05:41

bors mentioned this pull request Sep 12, 2017

Implement RFC 1861: Extern types #44295

Merged

bors merged commit 57ebd28 into rust-lang:master Sep 12, 2017

eddyb deleted the deferred-ctfe branch September 12, 2017 06:50

This was referenced Sep 12, 2017

rustc: Make CrateStore private to TyCtxt #44420

Merged

Analyse storage liveness and preserve it during generator transformation #44480

Merged

QuietMisdreavus mentioned this pull request Sep 13, 2017

rustdoc: fixed arrays are printing with const-eval debug info #44555

Closed

alexcrichton mentioned this pull request Sep 15, 2017

Up to 8% RSS regression because of #44142 #44576

Closed

eddyb mentioned this pull request Sep 15, 2017

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

alexcrichton mentioned this pull request Sep 18, 2017

7% regression in compile time of tuple-stress benchmark #44677

Closed

Zalathar mentioned this pull request Aug 17, 2024

Remove a useless ref/id/ref round-trip from pattern_from_hir #129196

Merged

Evaluate fixed-length array length expressions lazily. #44275

Evaluate fixed-length array length expressions lazily. #44275

Conversation

eddyb commented Sep 2, 2017

eddyb commented Sep 2, 2017

kennytm commented Sep 2, 2017 • edited Loading

eddyb commented Sep 2, 2017

kennytm commented Sep 2, 2017 • edited Loading

eddyb commented Sep 2, 2017 • edited Loading

eddyb commented Sep 2, 2017

eddyb commented Sep 2, 2017

eddyb commented Sep 3, 2017

Mark-Simulacrum left a 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

bors commented Sep 3, 2017

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

eddyb Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Sep 6, 2017

bors commented Sep 6, 2017

eddyb commented Sep 6, 2017

bors commented Sep 6, 2017

Mark-Simulacrum commented Sep 6, 2017

eddyb commented Sep 6, 2017

Mark-Simulacrum commented Sep 6, 2017

Mark-Simulacrum commented Sep 6, 2017

nikomatsakis left a 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

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

bors commented Sep 8, 2017

bors commented Sep 11, 2017

nikomatsakis commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 12, 2017

bors commented Sep 12, 2017

kennytm commented Sep 2, 2017 •

edited

Loading

kennytm commented Sep 2, 2017 •

edited

Loading

eddyb commented Sep 2, 2017 •

edited

Loading

eddyb Sep 7, 2017 •

edited

Loading