Add forever unstable attribute to allow specifying arbitrary scalar ranges by oli-obk · Pull Request #54032 · rust-lang/rust

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

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

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

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

+#[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.

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

 @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

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


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

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

+ 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/github.com-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.

constFOO_A:u32 = 0xFFFF_FFFF;constFOO_B:u32 = 0xFFFF_FFFE;structFoo{u:u32}

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

to

#[rustc_layout_scalar_range_end(0xFFFF_FF00)]structBar(u32);enumFoo{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

Comment threadsrc/librustc/ty/context.rs Outdated
eddybSep 8, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the RangeBound enum from libstd instead of Option here.

oli-obk force-pushed the layout_scalar_ranges branch from 7e185fc to 95987d0Compare 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 8053f63Compare September 11, 2018 10:07

eddyb reviewed Sep 11, 2018
View reviewed changes

Comment threadsrc/librustc/middle/region.rs

eddyb reviewed Sep 11, 2018
View reviewed changes

Comment threadsrc/librustc/middle/region.rs

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:masterSep 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

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

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

Uh oh!

Uh oh!

Conversation

oli-obk commented Sep 7, 2018

Uh oh!

nagisaSep 7, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obkSep 7, 2018

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddybSep 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddybSep 8, 2018

Choose a reason for hiding this comment

Uh oh!

eddyb commented Sep 8, 2018

Uh oh!

eddybSep 11, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eddybSep 11, 2018

Choose a reason for hiding this comment

Uh oh!

eddybSep 11, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

oli-obk commented Sep 11, 2018

Uh oh!

oli-obk commented Sep 11, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Sep 11, 2018

Uh oh!

This comment has been minimized.

eddybSep 11, 2018

Choose a reason for hiding this comment

Uh oh!

eddyb commented Sep 11, 2018

Uh oh!

bors commented Sep 11, 2018

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

bors commented Sep 14, 2018

Uh oh!

bors commented Sep 14, 2018

Uh oh!

nnethercote commented Sep 22, 2018

Uh oh!

oli-obk commented Sep 22, 2018

Uh oh!

nnethercote commented Sep 25, 2018

Uh oh!

oli-obk commented Oct 1, 2018

Uh oh!

oli-obk commented Oct 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

eddybSep 8, 2018•
edited
Loading

oli-obk commented Sep 11, 2018•
edited
Loading