Use ValTree in all type level constants #83234

oli-obk · 2021-03-17T16:52:18Z

based on #83207

best reviewed commit by commit, as some of the refactoring commits are noisy but don't change anything significant.

cc @rust-lang/wg-const-eval

Type level constants are now fully backed by value trees, so it is impossible for two constants to have the same value but a different representation.

oli-obk · 2021-03-17T16:54:19Z

compiler/rustc_middle/src/ty/relate.rs

-        (ty::ConstKind::Value(a_val), ty::ConstKind::Value(b_val)) => {
-            check_const_value_eq(relation, a_val, b_val, a, b)?
-        }
+        (ty::ConstKind::Value(a_val), ty::ConstKind::Value(b_val)) => a_val == b_val,


This is the essence of this PR. There is no more special logic for relating constants, we can just compare them with the derived PartialEq impl.

oli-obk · 2021-03-17T16:55:36Z

src/test/mir-opt/const_prop/control_flow_simplification.hello.ConstProp.diff

-                                           // + val: Value(Slice { data: Allocation { bytes: [101, 120, 112, 108, 105, 99, 105, 116, 32, 112, 97, 110, 105, 99], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [16383], len: Size { raw: 14 } }, size: Size { raw: 14 }, align: Align { pow2: 0 }, mutability: Not, extra: () }, start: 0, end: 14 })
+                                           // + val: Value(Branch([Leaf(0x65), Leaf(0x78), Leaf(0x70), Leaf(0x6c), Leaf(0x69), Leaf(0x63), Leaf(0x69), Leaf(0x74), Leaf(0x20), Leaf(0x70), Leaf(0x61), Leaf(0x6e), Leaf(0x69), Leaf(0x63)]))
                                           // mir::Constant
                                           // + span: $SRC_DIR/std/src/panic.rs:LL:COL
-                                           // + literal: Const { ty: &str, val: Value(Slice { data: Allocation { bytes: [101, 120, 112, 108, 105, 99, 105, 116, 32, 112, 97, 110, 105, 99], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [16383], len: Size { raw: 14 } }, size: Size { raw: 14 }, align: Align { pow2: 0 }, mutability: Not, extra: () }, start: 0, end: 14 }) }
+                                           // + literal: Const { ty: &str, val: Value(Branch([Leaf(0x65), Leaf(0x78), Leaf(0x70), Leaf(0x6c), Leaf(0x69), Leaf(0x63), Leaf(0x69), Leaf(0x74), Leaf(0x20), Leaf(0x70), Leaf(0x61), Leaf(0x6e), Leaf(0x69), Leaf(0x63)])) }


we should probably stop printing string constants in mir dumps.. their stringly printed representation is sufficient

Can we tie this to -Zverbose or so?

oli-obk · 2021-03-17T16:56:41Z

src/test/mir-opt/if_condition_int.dont_remove_comparison.SimplifyComparisonIntegral.diff

-         _2 = Eq(move _3, const 17_i8);   // scope 0 at $DIR/if-condition-int.rs:44:13: 44:20
-         StorageDead(_3);                 // scope 0 at $DIR/if-condition-int.rs:44:19: 44:20
-         switchInt(_2) -> [false: bb2, otherwise: bb1]; // scope 1 at $DIR/if-condition-int.rs:46:9: 46:14
-+         _2 = Eq(_3, const 17_i8);        // scope 0 at $DIR/if-condition-int.rs:44:13: 44:20
-+         nop;                             // scope 0 at $DIR/if-condition-int.rs:44:19: 44:20
-+         switchInt(move _3) -> [17_i8: bb1, otherwise: bb2]; // scope 1 at $DIR/if-condition-int.rs:46:9: 46:14
+          _2 = Eq(move _3, const 17_i8);   // scope 0 at $DIR/if-condition-int.rs:44:13: 44:20
+          StorageDead(_3);                 // scope 0 at $DIR/if-condition-int.rs:44:19: 44:20
+          switchInt(_2) -> [false: bb2, otherwise: bb1]; // scope 1 at $DIR/if-condition-int.rs:46:9: 46:14


This one is interesting... this change by itself enabled an optimization to trigger more often

oli-obk · 2021-03-17T16:57:29Z

src/test/ui/suggestions/const-pat-non-exaustive-let-new-var.rs

@@ -1,6 +1,6 @@
 fn main() {
    let A = 3;
-    //~^ ERROR refutable pattern in local binding: `i32::MIN..=1_i32` and
+    //~^ ERROR refutable pattern in local binding: `i32::MIN..=1` and


This change is because we're now using ConstInt instead of ty::Const printing for these errors

compiler/rustc_middle/src/mir/mod.rs

bors · 2021-03-17T22:53:57Z

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

oli-obk · 2021-03-19T18:09:53Z

@bors try @rust-timer queue

rust-timer · 2021-03-19T18:09:54Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-03-19T18:10:03Z

⌛ Trying commit 5cf5addca286bfde62041568f38117ceafc84e4c with merge 437ebfc4e9d5c202057e1c2509e25c70d2fc67eb...

bors · 2021-03-19T18:57:31Z

☀️ Try build successful - checks-actions
Build commit: 437ebfc4e9d5c202057e1c2509e25c70d2fc67eb (437ebfc4e9d5c202057e1c2509e25c70d2fc67eb)

rust-timer · 2021-03-19T18:57:33Z

Queued 437ebfc4e9d5c202057e1c2509e25c70d2fc67eb with parent b97fd3e, future comparison URL.

rust-timer · 2021-03-19T22:01:27Z

Finished benchmarking try commit (437ebfc4e9d5c202057e1c2509e25c70d2fc67eb): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

oli-obk · 2021-03-20T13:14:42Z

oof. that's some results... Some good, some very bad results (stress tests). I gotta analyze the stress tests to see which part is problematic. It may be string related, as strings are now encoded as valtree branches (so one valtree leaf per byte in the string), making them rather expensive to be converted back into allocations. Since one of the tests does this a lot, that may just be the root of the regression. Not sure how to address this though. Ideally we would just have a Str(Symbol) variant. This works just fine for str, as there is not equivalent valtree representation that could conflict. But we can't do the same for byte strings, as they really are just arrays of bytes, which already have a defined representation in valtrees (namely a branch full of single-char leafs).

I'll experiment with just fixing strings for now and we'll see whether we can at least not regress our benchmarks, even if we'll still have a regression for byte strings (but no benchmark for them).

lcnr · 2021-05-24T18:17:39Z

I am not able to review any PRs in the near future.

We need both impl work (as @oli-obk is also on a break) and review for this PR, so it probably doesn't make much sense to reassign, but

r? @varkor

…:Const

...instead of carrying around ty::Const and erroring later

…aration of value trees making such operations on `ty::Const` obsolete.

…woerister,oli-obk rustc_symbol_mangling: support structural constants and &str in v0. This PR should unblock rust-lang#85530 (except for float `const` generics, which AFAIK should've never worked). (cc `@tmiasko` could the rust-lang#85530 (comment) failures be retried with a quick crater "subset" run of this PR + changing the default to `v0`? Just to make sure I didn't miss anything other than the floats) The encoding is the one suggested before in e.g. rust-lang#61486 (comment), tho this PR won't by itself finish rust-lang#61486, before closing that we'd likely want to move to `@oli-obk's` "valtrees" (i.e. rust-lang#83234 and other associated work). <hr> **EDITs**: 1. switched unit/tuple/braced-with-named-fields `<const-fields>` prefixes from `"u"`/`"T"`/`""` to `"U"`/`"T"`/`"S"` to avoid the ambiguity reported by `@tmiasko` in rust-lang#87194 (comment). 2. `rustc-demangle` PR: rust-lang/rustc-demangle#55 3. RFC amendment PR: rust-lang/rfcs#3161 * also removed the grammar changes included in that PR, from this description 4. added tests (temporarily using my fork of `rustc-demangle`) <hr> r? `@michaelwoerister`

JohnCSimon · 2021-09-12T07:39:24Z

triage: merge conflicts

oli-obk · 2021-10-05T10:20:44Z

I'm going to close this for now. I'm going to need another few months before I get to this, so we don't need to keep it open

tshepang · 2022-08-10T18:33:59Z

see #96591

rust-highfive assigned lcnr Mar 17, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2021

oli-obk commented Mar 17, 2021

View reviewed changes