don't allow ZST in ScalarInt #98957

RalfJung · 2022-07-05T19:43:32Z

There are several indications that we should not ZST as a ScalarInt:

We had two ways to have ZST valtrees, either an empty Branch or a Leaf with a ZST in it.
ValTree::zst() used the former, but the latter could possibly arise as well.
Likewise, the interpreter had Immediate::Uninit and Immediate::Scalar(Scalar::ZST).
LLVM codegen already had to special-case ZST ScalarInt.

So I propose we stop using ScalarInt to represent ZST (which are clearly not integers). Instead, we can add new ZST variants to those types that did not have other variants which could be used for this purpose.

Based on #98831. Only the commits starting from "don't allow ZST in ScalarInt" are new.

r? @oli-obk

rustbot · 2022-07-05T19:43:36Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in const_evaluatable.rs

cc @lcnr

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_codegen_llvm/src/common.rs

compiler/rustc_middle/src/mir/interpret/value.rs

lcnr · 2022-07-05T19:52:23Z

compiler/rustc_middle/src/mir/pretty.rs

@@ -449,6 +449,7 @@ impl<'tcx> Visitor<'tcx> for ExtraComments<'tcx> {
            }

            let fmt_val = |val: &ConstValue<'tcx>| match val {
+                ConstValue::ZST => format!("ZST"),


nit: the value isn't really a zero sized type 😁 idk what else I would want to print here though

probably <ZST> which is what we've done for zero sized scalars before?

Yeah, it's a ZSV, a zero-sized value. But we have called the value a ZST for a while now... (there's some red diff in this PR removing a <ZST> elsewhere; I think this will have to cause some test failure somewhere)

If they're not too long, ConstValue::ZeroSized or ConstValue::UnitLike both seem fine to me.

compiler/rustc_middle/src/thir.rs

RalfJung · 2022-07-05T20:47:11Z

What's up with CI? I checked this locally...
... oh, it's in cranelift.

rustbot · 2022-07-05T20:53:55Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

bjorn3 · 2022-07-06T11:30:16Z

compiler/rustc_codegen_cranelift/src/constant.rs

@@ -167,6 +167,7 @@ pub(crate) fn codegen_const_value<'tcx>(
    }

    match const_val {
+        ConstValue::ZST => unreachable!(), // we already handles ZST above


Does layout.is_zst() always imply ConstValue::ZST? If so moving CValue::by_ref(crate::Pointer::dangling(layout.align.pref), layout) into this branch will be a tiny bit faster at runtime and reduce compile time of cg_clif itself a bit too.

Does layout.is_zst() always imply ConstValue::ZST?

I don't know. There might be by-ref ZST ConstValues.

src/test/mir-opt/array_index_is_temporary.main.SimplifyCfg-elaborate-drops.after.32bit.mir

src/test/mir-opt/nll/region_subtyping_basic.main.nll.0.32bit.mir

RalfJung · 2022-07-06T15:19:48Z

src/test/mir-opt/lower_intrinsics.f_unit.PreCodegen.before.mir

        scope 2 (inlined std::mem::size_of::<()>) { // at $DIR/lower_intrinsics.rs:45:8: 45:32
        }
    }

    bb0: {
        StorageLive(_1);                 // scope 0 at $DIR/lower_intrinsics.rs:34:16: 34:18
        StorageLive(_2);                 // scope 1 at $DIR/lower_intrinsics.rs:46:9: 46:17
-        _2 = f_zst::<()>(const ()) -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:46:9: 46:17
+        StorageLive(_3);                 // scope 1 at $DIR/lower_intrinsics.rs:46:15: 46:16
+        _2 = f_zst::<()>(move _3) -> bb1; // scope 1 at $DIR/lower_intrinsics.rs:46:9: 46:17


No idea why this is not const-propped any more.

no idea either, but this could have happened with more complex logic before, too, so it's fine to regress and we'll just have to fix const prop

bors · 2022-07-06T18:20:12Z

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

RalfJung · 2022-07-07T02:22:33Z

Conflicts resolved and all ready. @oli-obk is that const-prop change a problem or expected and fine? If it even is a const-prop change, the pass name says "lower-intrinsics", so maybe const-prop will fix this up again later?

oli-obk · 2022-07-07T09:15:27Z

@bors r=lcnr,oli-obk

bors · 2022-07-07T09:15:28Z

📌 Commit d1dd4879125325c6730f7f40f75e0b28851e9649 has been approved by lcnr,oli-obk

RalfJung · 2022-07-09T11:43:13Z

Oh, that test is needs-profiler-support. I don't think I can bless this one. I guess I have to do it by hand?

RalfJung · 2022-07-09T11:44:20Z

@bors r=lcnr,oli-obk rollup=never

bors · 2022-07-09T11:44:22Z

📌 Commit 4e7aaf1 has been approved by lcnr,oli-obk

It is now in the queue for this repository.

bors · 2022-07-09T17:16:03Z

⌛ Testing commit 4e7aaf1 with merge f893495...

bors · 2022-07-09T19:57:08Z

☀️ Test successful - checks-actions
Approved by: lcnr,oli-obk
Pushing f893495 to master...

rust-timer · 2022-07-09T22:22:35Z

Finished benchmarking commit (f893495): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	4.4%	4.4%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.4%	-0.4%	2
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-5.1%	-5.1%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	7.4%	11.2%	7
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-4.0%	-5.4%	5
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

RalfJung · 2022-07-09T22:45:59Z

Oh, that's pretty surprising. It's "only" a secondary benchmark so maybe it's okay...? We get some nice RSS reductions in secondary benchmarks in exchange, it seems. ;)

RalfJung · 2022-07-11T21:12:20Z

FWIW that benchmark has a "large amount of extern functions". I am not quite sure where our Scalar types even come to bear here, but it's probably values of FnDef type, which are now a ConstValue::ZeroSized rather than a scalar integer. I don't see any reason why that would be slower though... and indeed for "full" builds it is slightly faster, but for "incr-full" builds it is slower.

Supposedly, previously there were some NonHirLiteral involved, which used ty::ValTree::from_scalar_int, creating a valtree leaf. Now instead they use ty::ValTree::zst creating a valtree branch with 0 children. This is strictly more correct, we don't want to have two different ways of representing the same thing as a valtree (that would be unsound, probably). But looking at those functions there is really no reason why ty::ValTree::zst should be any slower... maybe it just needs an inline(always)?

I'll wait for the regular perf triage to come visit this PR and tell me whether this is okay or whether we should dig a bit deeper into what is going on.

oli-obk · 2022-07-12T07:09:40Z

  252,778,417  ???:<rustc_middle::dep_graph::dep_node::DepKind as rustc_query_system::dep_graph::DepKind>::read_deps::<<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::read_index::{closure
 -171,114,000  ???:<rustc_metadata::rmeta::decoder::cstore_impl::provide::{closure
      725,369  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::dep_node_exists
     -425,283  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::with_task::<rustc_middle::ty::context::TyCtxt, rustc_span::def_id::DefId, bool>
     -246,123  ???:<rustc_middle::hir::provide::{closure
     -107,395  ???:<rustc_query_system::dep_graph::graph::CurrentDepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::intern_node
      -90,051  ???:<alloc::raw_vec::RawVec<rustc_parse::parser::TokenCursorFrame>>::reserve_for_push

looks like inliner noise to me

eddyb · 2022-09-06T13:56:08Z

compiler/rustc_codegen_llvm/src/common.rs

    fn scalar_to_backend(&self, cv: Scalar, layout: abi::Scalar, llty: &'ll Type) -> &'ll Value {
        let bitsize = if layout.is_bool() { 1 } else { layout.size(self).bits() };
        match cv {
-            Scalar::Int(ScalarInt::ZST) => {
-                assert_eq!(0, layout.size(self).bytes());


So I'm looking at this (in the context of Rust-GPU updating the nightly version of Rust we use).

How could this ever pass? abi::Scalar just can't encode sizes other than 1, 2, 4, 8, 16:

rust/compiler/rustc_target/src/abi/mod.rs

Lines 605 to 611 in 3c72788

pub enum Integer {

I8,

I16,

I32,

I64,

I128,

}

rust/compiler/rustc_target/src/abi/mod.rs

Lines 703 to 715 in 3c72788

pub enum Primitive {

/// The `bool` is the signedness of the `Integer` type.

///

/// One would think we would not care about such details this low down,

/// but some ABIs are described in terms of C types and ISAs where the

/// integer arithmetic is done on {sign,zero}-extended registers, e.g.

/// a negative integer passed by zero-extension will appear positive in

/// the callee, and most operations on it will produce the wrong values.

Int(Integer, bool),

F32,

F64,

Pointer,

}

rust/compiler/rustc_target/src/abi/mod.rs

Lines 827 to 844 in 3c72788

pub enum Scalar {

Initialized {

value: Primitive,

// FIXME(eddyb) always use the shortest range, e.g., by finding

// the largest space between two consecutive valid values and

// taking everything else as the (shortest) valid range.

valid_range: WrappingRange,

},

Union {

/// Even for unions, we need to use the correct registers for the kind of

/// values inside the union, so we keep the `Primitive` type around. We

/// also use it to compute the size of the scalar.

/// However, unions never have niches and even allow undef,

/// so there is no `valid_range`.

value: Primitive,

},

}

This is the const eval Scalar, not abi Scalar.

layout is abi::Scalar though.

If you scroll up through this PR there is a comment of mine somewhere saying that this must have been dead code.

eddyb · 2022-09-06T13:57:01Z

compiler/rustc_codegen_llvm/src/common.rs

@@ -223,13 +222,13 @@ impl<'ll, 'tcx> ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
        })
    }

+    fn zst_to_backend(&self, _llty: &'ll Type) -> &'ll Value {
+        self.const_undef(self.type_ix(0))


I don't think i0 is a legal LLVM type? At least not according to https://llvm.org/docs/LangRef.html#integer-type

I copied this from elsewhere... 🤷

eddyb · 2022-09-06T13:57:53Z

compiler/rustc_codegen_ssa/src/mir/operand.rs

+            ConstValue::ZeroSized => {
+                let llval = bx.zst_to_backend(bx.immediate_backend_type(layout));
+                OperandValue::Immediate(llval)
+            }


If you expand the diff there's a if layout.is_zst() { above, that early-returns, so this should've been unreachable!() without the illegal zst_to_backend implementations existing, looks like.
(and this is the only callsite of zst_to_backend AFAICT)

Though I think I see how this happened, scalar_to_backend had a nonsense Scalar::Int(ScalarInt::ZST) => { case from many years ago (at least 4? I wasn't able to easily go far back enough in git blame).

And this PR just refactored it into a more obvious spot.

Remove dead broken code from const zst handling in backends cc `@RalfJung` found by `@eddyb` in rust-lang#98957 (comment)

rust-highfive assigned oli-obk Jul 5, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 5, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2022

RalfJung commented Jul 5, 2022

View reviewed changes

compiler/rustc_codegen_llvm/src/common.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

lcnr reviewed Jul 5, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the zst-are-different branch from 3fa1a07 to 97b345b Compare July 5, 2022 22:52

This comment has been minimized.

Sign in to view

RalfJung force-pushed the zst-are-different branch from 97b345b to 943041a Compare July 5, 2022 23:56

bjorn3 reviewed Jul 6, 2022

View reviewed changes

src/test/mir-opt/array_index_is_temporary.main.SimplifyCfg-elaborate-drops.after.32bit.mir Outdated Show resolved Hide resolved

RalfJung added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2022

RalfJung force-pushed the zst-are-different branch from 943041a to c5f27a9 Compare July 6, 2022 14:57

RalfJung commented Jul 6, 2022

View reviewed changes

src/test/mir-opt/nll/region_subtyping_basic.main.nll.0.32bit.mir Outdated Show resolved Hide resolved

RalfJung commented Jul 6, 2022

View reviewed changes

RalfJung force-pushed the zst-are-different branch from c5f27a9 to 6ec9abe Compare July 6, 2022 15:24

RalfJung force-pushed the zst-are-different branch from 6ec9abe to d1dd487 Compare July 7, 2022 01:59

oli-obk approved these changes Jul 7, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 7, 2022

tweak names and output and bless

4e7aaf1

RalfJung force-pushed the zst-are-different branch from 9864f36 to 4e7aaf1 Compare July 9, 2022 11:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 9, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 9, 2022

bors merged commit f893495 into rust-lang:master Jul 9, 2022

rustbot added this to the 1.64.0 milestone Jul 9, 2022

This was referenced Jul 9, 2022

Add runtime validity checks inside MaybeUninit::assume_init #98073

Closed

Pull Derefer before ElaborateDrops #98145

Merged

Move abstract const to middle #99000

Merged

rustbot added the perf-regression Performance regression. label Jul 9, 2022

RalfJung deleted the zst-are-different branch July 9, 2022 22:39

RalfJung mentioned this pull request Jul 11, 2022

perf check: inline valtree constructors #99166

Closed

celinval mentioned this pull request Jul 21, 2022

Update toolchain to nightly-2022-07-19 model-checking/kani#1399

Merged

4 tasks

eddyb reviewed Sep 6, 2022

View reviewed changes

oli-obk mentioned this pull request Sep 6, 2022

Remove dead broken code from const zst handling in backends #101484

Merged

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Sep 6, 2022

Rollup merge of rust-lang#101484 - oli-obk:no_zst, r=eddyb

1d49dce

Remove dead broken code from const zst handling in backends cc `@RalfJung` found by `@eddyb` in rust-lang#98957 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't allow ZST in ScalarInt #98957

don't allow ZST in ScalarInt #98957

RalfJung commented Jul 5, 2022 •

edited

Loading

rustbot commented Jul 5, 2022

This comment has been minimized.

lcnr Jul 5, 2022

RalfJung Jul 5, 2022

eddyb Jul 7, 2022

RalfJung commented Jul 5, 2022

rustbot commented Jul 5, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bjorn3 Jul 6, 2022

RalfJung Jul 6, 2022

RalfJung Jul 6, 2022

oli-obk Jul 7, 2022

bors commented Jul 6, 2022

RalfJung commented Jul 7, 2022

oli-obk commented Jul 7, 2022

bors commented Jul 7, 2022

RalfJung commented Jul 9, 2022

RalfJung commented Jul 9, 2022

bors commented Jul 9, 2022

bors commented Jul 9, 2022

bors commented Jul 9, 2022

rust-timer commented Jul 9, 2022

RalfJung commented Jul 9, 2022

RalfJung commented Jul 11, 2022 •

edited

Loading

oli-obk commented Jul 12, 2022

eddyb Sep 6, 2022

bjorn3 Sep 6, 2022

RalfJung Sep 6, 2022

eddyb Sep 6, 2022

RalfJung Sep 6, 2022

eddyb Sep 6, 2022 •

edited

Loading

eddyb Sep 6, 2022

	pub enum Primitive {
	/// The `bool` is the signedness of the `Integer` type.
	///
	/// One would think we would not care about such details this low down,
	/// but some ABIs are described in terms of C types and ISAs where the
	/// integer arithmetic is done on {sign,zero}-extended registers, e.g.
	/// a negative integer passed by zero-extension will appear positive in
	/// the callee, and most operations on it will produce the wrong values.
	Int(Integer, bool),
	F32,
	F64,
	Pointer,
	}

	pub enum Scalar {
	Initialized {
	value: Primitive,

	// FIXME(eddyb) always use the shortest range, e.g., by finding
	// the largest space between two consecutive valid values and
	// taking everything else as the (shortest) valid range.
	valid_range: WrappingRange,
	},
	Union {
	/// Even for unions, we need to use the correct registers for the kind of
	/// values inside the union, so we keep the `Primitive` type around. We
	/// also use it to compute the size of the scalar.
	/// However, unions never have niches and even allow undef,
	/// so there is no `valid_range`.
	value: Primitive,
	},
	}

don't allow ZST in ScalarInt #98957

don't allow ZST in ScalarInt #98957

Conversation

RalfJung commented Jul 5, 2022 • edited Loading

rustbot commented Jul 5, 2022

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 5, 2022

rustbot commented Jul 5, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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 Jul 6, 2022

RalfJung commented Jul 7, 2022

oli-obk commented Jul 7, 2022

bors commented Jul 7, 2022

RalfJung commented Jul 9, 2022

RalfJung commented Jul 9, 2022

bors commented Jul 9, 2022

bors commented Jul 9, 2022

bors commented Jul 9, 2022

rust-timer commented Jul 9, 2022

Footnotes

RalfJung commented Jul 9, 2022

RalfJung commented Jul 11, 2022 • edited Loading

oli-obk commented Jul 12, 2022

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

eddyb Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 5, 2022 •

edited

Loading

RalfJung commented Jul 11, 2022 •

edited

Loading

eddyb Sep 6, 2022 •

edited

Loading