Add forever unstable attribute to allow specifying arbitrary scalar ranges #54032

oli-obk · 2018-09-07T14:47:46Z

r? @eddyb for the first commit and @nikomatsakis for the second one

nagisa · 2018-09-07T14:58:44Z

src/librustc/middle/region.rs

@@ -161,8 +161,7 @@ pub struct BlockRemainder {

 newtype_index!(FirstStatementIndex
    {
-        pub idx
-        MAX = SCOPE_DATA_REMAINDER_MAX
+        MAX = 0xFFFF_FFFB // the remainder of the scope data


Can we do something less magic-value like than this? 0xFFFF_FFFB is two steps away from the values in SCOPE_DATA_*. Changing the definition of SCOPE_DATA_* to be 0xFFFF_FFFE, ... 0xFFFF_FFFD would help somewhat, if it is not possible to use arithmetic in the attribute.

SCOPE_DATA_DESTRUCTION is 0xFFFF_FFFC, so this is exactly next to it.

Attributes currently only allow literals, no expressions. Though referring to constants might work via queries, there's currently no support for that.

eddyb · 2018-09-08T17:13:46Z

src/librustc/middle/region.rs

+const SCOPE_DATA_CALLSITE: u32 = 0xFFFF_FFFE;
+const SCOPE_DATA_ARGUMENTS: u32 = 0xFFFF_FFFD;
+const SCOPE_DATA_DESTRUCTION: u32 = 0xFFFF_FFFC;
+// be sure to add the MAX of FirstStatementIndex if you add more constants here


Why isn't this done by having an enum with 4 unit variants and a tuple variant around FirstStatementIndex?

eddyb · 2018-09-08T17:14:31Z

src/librustc/ty/context.rs

@@ -1083,6 +1083,29 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
        interned
    }

+    /// Returns a range of the start/end indices specified with the `rustc_layout_scalar_range`
+    /// attribute. Missing range ends may be denoted by `None` and will just use the max/min of
+    /// the type.


You can use the RangeBound enum from libstd instead of Option here.

eddyb · 2018-09-08T17:16:37Z

@oli-obk I think the name should have valid_range in it, "scalar" is not enough IMO.

…ifications

…imizations

… the MAX

…e increases

eddyb · 2018-09-11T10:14:35Z

src/librustc/middle/region.rs

+#[allow(dead_code)]
+// only works on stage 1 when the rustc_layout_scalar_valid_range attribute actually exists
+#[cfg(not(stage0))]
+static ASSERT: () = [()][(mem::size_of::<ScopeData>() != 4) as usize];


I would put ! around the expression and use == instead of !=. That way, it reads more like an assert.

src/librustc/middle/region.rs

eddyb · 2018-09-11T10:16:45Z

src/librustc_data_structures/indexed_vec.rs

@@ -82,7 +83,8 @@ macro_rules! newtype_index {
        newtype_index!(
            // Leave out derives marker so we can use its absence to ensure it comes first
            @type         [$name]
-            @max          [::std::u32::MAX - 1]
+            // shave off 256 indices at the end to allow space for packing these indices into enums
+            @max          [0xFFFF_FF00]


Ohhhh, this is clever!

eddyb · 2018-09-11T10:17:40Z

src/test/ui/consts/const-eval/zst_operand_eval.rs

@@ -0,0 +1,5 @@
+// compile-pass
+
+static ASSERT: () = [()][(std::mem::size_of::<u32>() != 4) as usize];


I'd make the same change here (! around ==).

src/librustc/middle/region.rs

oli-obk · 2018-09-11T12:47:48Z

Ups forgot to push. Stage 2 tests are passing locally now.

oli-obk · 2018-09-11T12:56:23Z

I'm presuming

@bors r=eddyb

since all review comments have been addressed

bors · 2018-09-11T12:57:14Z

📌 Commit 79f2cc0 has been approved by eddyb

eddyb · 2018-09-11T19:50:15Z

src/librustc/middle/region.rs

+            ScopeData::Destruction => write!(fmt, "Destruction({:?})", self.id),
+            ScopeData::Remainder(fsi) => write!(
+                fmt,
+                "Remainder {{ block: {:?}, first_statement_index: {}}}",


You probably want to use the fmt.debug_struct wrapper, to automate this.

eddyb · 2018-09-11T19:52:26Z

@bors r+

bors · 2018-09-11T19:52:27Z

📌 Commit a94c166 has been approved by eddyb

nikomatsakis

OK, I was wrong. This is awesome! Nice work @oli-obk! 🎉

bors · 2018-09-14T09:47:27Z

⌛ Testing commit a94c166 with merge dfabe4b...

@eddyb

Add forever unstable attribute to allow specifying arbitrary scalar ranges r? @eddyb for the first commit and @nikomatsakis for the second one

bors · 2018-09-14T12:09:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing dfabe4b to master...

Make rustc::middle::region::Scope's fields public This PR makes the following changes to `rustc::middle::region::Scope`: - [x] Makes `region::Scope`'s fields public - [x] Removes the `impl Scope` block with constructors (as per [this comment](#54032 (comment))) - [x] Updates call sites throughout the compiler Closes #54122.

nnethercote · 2018-09-22T03:00:17Z

This hurt compile speed by up to 2% across a lot of benchmarks:
https://perf.rust-lang.org/compare.html?start=fccde0018a618eb6f45d2a3c97f629809994dff6&end=dfabe4b88509bfff1eca9060ccc9a4d28ee394e8&stat=instructions:u

@oli-obk: any thoughts?

oli-obk · 2018-09-22T10:38:49Z

Previously the unpacking was done manually. Now ruatc generates the relevant enum matching code. Maybe it's not as optimizeable?

nnethercote · 2018-09-25T03:31:13Z

@oli-obk: Here is a Cachegrind diff comparing instruction counts before vs. after. Positive numbers means the instruction counts went up, negative numbers mean they went down.

 9,254,752  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:<rustc::ty::Predicate<'tcx> as core::cmp::PartialEq>::eq
 9,105,881  /home/njn/moz/rustN/src/libcore/cell.rs:rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
 6,810,037  /home/njn/moz/rustN/src/librustc/ty/mod.rs:<rustc::ty::Predicate<'tcx> as core::hash::Hash>::hash
 5,633,067  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
 5,153,016  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:rustc::traits::select::SelectionContext::assemble_candidates
 4,295,696  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:<rustc::ty::Predicate<'tcx> as core::hash::Hash>::hash
 4,130,136  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:<rustc::ty::sty::TyKind<'tcx> as core::cmp::PartialEq>::eq
 3,785,310  /home/njn/moz/rustN/src/libcore/num/mod.rs:<rustc_data_structures::snapshot_map::SnapshotMap<K, V>>::get
-3,703,491  /home/njn/moz/rustN/src/librustc/ty/query/plumbing.rs:rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
 2,583,451  /home/njn/moz/rustN/src/libcore/num/mod.rs:<rustc::ty::Predicate<'tcx> as core::hash::Hash>::hash
 2,396,730  /home/njn/moz/rustN/src/librustc_data_structures/snapshot_map/mod.rs:<rustc_data_structures::snapshot_map::SnapshotMap<K, V>>::get
-2,392,645  /home/njn/moz/rustN/src/libcore/num/mod.rs:rustc::traits::project::opt_normalize_projection_type
 2,151,640  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:<rustc_data_structures::snapshot_map::SnapshotMap<K, V>>::get
 2,056,569  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:rustc::infer::higher_ranked::<impl rustc::infer::combine::CombineFields<'a, 'gcx, 'tcx>>::higher_ranked_sub::{{closure}}
 2,056,161  /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:<rustc_data_structures::snapshot_map::SnapshotMap<K, V>>::get
-2,041,430  /home/njn/moz/rustN/src/libcore/hash/mod.rs:<rustc::ty::Predicate<'tcx> as core::hash::Hash>::hash
-1,794,496  /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc::traits::project::opt_normalize_projection_type
 1,677,995  /home/njn/moz/rustN/src/libcore/ptr.rs:<rustc_data_structures::snapshot_map::SnapshotMap<K, V>>::get
-1,630,473  /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:rustc::traits::project::opt_normalize_projection_type
 1,597,264  /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:<rustc_data_structures::snapshot_map::SnapshotMap<K, V>>::get
 1,564,224  /home/njn/moz/rustN/src/librustc/middle/region.rs:rustc::middle::region::ScopeTree::temporary_scope
 1,544,676  /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
 1,528,640  /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
 1,381,946  /home/njn/moz/rustN/src/librustc/hir/def_id.rs:rustc::middle::lang_items::LanguageItems::fn_trait_kind
 1,278,687  /home/njn/moz/rustN/src/librustc/middle/region.rs:rustc::middle::region::ScopeTree::opt_encl_scope
 1,228,759  /home/njn/moz/rustN/src/librustc/traits/project.rs:rustc::traits::project::opt_normalize_projection_type
 1,173,536  /home/njn/moz/rustN/src/librustc/hir/mod.rs:<rustc::ty::sty::TyKind<'tcx> as core::hash::Hash>::hash
 1,136,358  /home/njn/.cargo/registry/src/suiyiyu.us.kg-1ecc6299db9ec823/rustc-hash-1.0.1/src/lib.rs:<rustc::ty::Predicate<'tcx> as core::hash::Hash>::hash
 1,022,004  /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc::middle::region::ScopeTree::opt_encl_scope

Any more thoughts about how to fix the regression?

oli-obk · 2018-10-01T11:58:39Z

I'll create some artificial benchmarks to try to repro the issue in isolation. Without knowing which case exactly is slow/verbose and we'd be poking in the dark.

oli-obk · 2018-10-13T07:04:49Z

I think I know the reason for this now. I changed e.g.

const FOO_A: u32 = 0xFFFF_FFFF;
const FOO_B: u32 = 0xFFFF_FFFE;
struct Foo { u: u32 }

https://play.rust-lang.org/?gist=639deede27237c3913e57ac4181f42bf&version=nightly&mode=debug&edition=2015

to

#[rustc_layout_scalar_range_end(0xFFFF_FF00)]
struct Bar(u32);
enum Foo {
    A,
    B,
    Other(Bar),
}

https://play.rust-lang.org/?gist=639deede27237c3913e57ac4181f42bf&version=nightly&mode=debug&edition=2015

While this will result in pretty much the same layout as before, any derived code on Foo will now generate less optimal code. Apparently llvm can't manage to clean that up.

The llvm IR for the second playground link is ca 150 LOC, the first link has 50 LOC

I'm opening an issue for this problem, since I can also create an example on stable code (without the unstable attribute) that will result in the same lack of optimitations

rust-highfive assigned eddyb Sep 7, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2018

nagisa reviewed Sep 7, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

eddyb reviewed Sep 8, 2018

View reviewed changes

oli-obk force-pushed the layout_scalar_ranges branch from 7e185fc to 95987d0 Compare September 10, 2018 12:40

oli-obk added 10 commits September 11, 2018 11:19

Get rid of the non_zero lang item in favour of arbitrary range spec…

d272e2f

…ifications

Forbid the upper indices of IndexVec indices to allow for niche opt…

06a041c

…imizations

Use the same way to specifiy the SCOPE_DATA_* constants as used for…

3828e9e

… the MAX

Satisfy tidy

a2c924b

Address attribute naming and use Bound enum

833dc7e

only allow restricting scalar layouts

88ebb95

Simplify Scope/ScopeData to have less chance of introducing UB or siz…

24dca6a

…e increases

Fix const eval of ZST index operations to make the static assertion work

d1b5231

Split CrateNum into an enum instead of having magic constants

1ec8670

Rebase fallout

8053f63

oli-obk force-pushed the layout_scalar_ranges branch from 2285e76 to 8053f63 Compare September 11, 2018 10:07

eddyb reviewed Sep 11, 2018

View reviewed changes

src/librustc/middle/region.rs Show resolved Hide resolved

eddyb reviewed Sep 11, 2018

View reviewed changes

src/librustc/middle/region.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

oli-obk mentioned this pull request Sep 11, 2018

Make rustc::middle::region::Scope's fields public... #54122

Closed

Use assertion-like static assertions

68298a5

This comment has been minimized.

Sign in to view

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2018

This comment has been minimized.

Sign in to view

eddyb reviewed Sep 11, 2018

View reviewed changes

Add missing #![feature(min_const_fn)] to the newtype_index test.

a94c166

nikomatsakis approved these changes Sep 13, 2018

View reviewed changes

bors added a commit that referenced this pull request Sep 14, 2018

Auto merge of #54032 - oli-obk:layout_scalar_ranges, r=eddyb

dfabe4b

Add forever unstable attribute to allow specifying arbitrary scalar ranges r? @eddyb for the first commit and @nikomatsakis for the second one

bors merged commit a94c166 into rust-lang:master Sep 14, 2018

bors mentioned this pull request Sep 14, 2018

don't elide lifetimes in paths in librustc/ #53816

Merged

maxdeviant mentioned this pull request Sep 15, 2018

Make rustc::middle::region::Scope's fields public #54260

Merged

3 tasks

oli-obk deleted the layout_scalar_ranges branch October 1, 2018 11:56

RalfJung mentioned this pull request Oct 5, 2018

Representation of unions rust-lang/unsafe-code-guidelines#13

Closed

cuviper mentioned this pull request Oct 31, 2018

1.30 beta.7 fails to build on ppc64el, "compiler unexpectedly panicked. this is a bug." #54545

Closed

SimonSapin mentioned this pull request Dec 26, 2018

Remove the private generic NonZero<T> wrapper type #57133

Merged

SimonSapin mentioned this pull request Jan 17, 2019

Add signed num::NonZeroI* types #57475

Merged

RalfJung mentioned this pull request Dec 21, 2019

Test slice patterns more #67467

Merged

cuviper mentioned this pull request Sep 1, 2020

change newtype_index! to use Nonzero #45842

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forever unstable attribute to allow specifying arbitrary scalar ranges #54032

Add forever unstable attribute to allow specifying arbitrary scalar ranges #54032

oli-obk commented Sep 7, 2018

nagisa Sep 7, 2018

oli-obk Sep 7, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb Sep 8, 2018 •

edited

Loading

eddyb Sep 8, 2018

eddyb commented Sep 8, 2018

eddyb Sep 11, 2018

eddyb Sep 11, 2018

eddyb Sep 11, 2018

This comment has been minimized.

This comment has been minimized.

oli-obk commented Sep 11, 2018

oli-obk commented Sep 11, 2018 •

edited

Loading

bors commented Sep 11, 2018

This comment has been minimized.

eddyb Sep 11, 2018

eddyb commented Sep 11, 2018

bors commented Sep 11, 2018

nikomatsakis left a comment

bors commented Sep 14, 2018

bors commented Sep 14, 2018

nnethercote commented Sep 22, 2018

oli-obk commented Sep 22, 2018

nnethercote commented Sep 25, 2018

oli-obk commented Oct 1, 2018

oli-obk commented Oct 13, 2018

		@@ -0,0 +1,5 @@
		// compile-pass

		static ASSERT: () = [()][(std::mem::size_of::<u32>() != 4) as usize];

Add forever unstable attribute to allow specifying arbitrary scalar ranges #54032

Add forever unstable attribute to allow specifying arbitrary scalar ranges #54032

Conversation

oli-obk commented Sep 7, 2018

nagisa Sep 7, 2018

Choose a reason for hiding this comment

oli-obk Sep 7, 2018

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

eddyb Sep 8, 2018

Choose a reason for hiding this comment

eddyb commented Sep 8, 2018

eddyb Sep 11, 2018

Choose a reason for hiding this comment

eddyb Sep 11, 2018

Choose a reason for hiding this comment

eddyb Sep 11, 2018

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

oli-obk commented Sep 11, 2018

oli-obk commented Sep 11, 2018 • edited Loading

bors commented Sep 11, 2018

This comment has been minimized.

eddyb Sep 11, 2018

Choose a reason for hiding this comment

eddyb commented Sep 11, 2018

bors commented Sep 11, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

bors commented Sep 14, 2018

bors commented Sep 14, 2018

nnethercote commented Sep 22, 2018

oli-obk commented Sep 22, 2018

nnethercote commented Sep 25, 2018

oli-obk commented Oct 1, 2018

oli-obk commented Oct 13, 2018

eddyb Sep 8, 2018 •

edited

Loading

oli-obk commented Sep 11, 2018 •

edited

Loading