Stream: t-compiler/wg-mir-opt

Topic: post christmas hack days


oli (Dec 21 2019 at 11:31, on Zulip):

Hi @WG-mir-opt Let's hack on mir opt things all day on 27/28 December. If you want to join, I set up https://meet.jit.si/wg-mir-opt and will be hanging out there starting around 9 or 10 CET on these days.

oli (Dec 21 2019 at 11:31, on Zulip):

Let's use this thread to figure out fun things for everyone to work on

oli (Dec 21 2019 at 11:32, on Zulip):

You're also totally welcome to join if you just want to hang out, there's no requirement to do any work.

Wesley Wiser (Dec 21 2019 at 15:22, on Zulip):

I'll join in around 10:30 CET after I've made some :coffee: :)

bjorn3 (Dec 21 2019 at 18:01, on Zulip):

It would be nice if #63802 (mir inliner panic) gets fixed.

Wesley Wiser (Dec 21 2019 at 23:06, on Zulip):

@bjorn3 It's on the list of things we're going to look at and I've already gotten a head start (see PR #67333)

Santiago Pastorino (Dec 22 2019 at 00:54, on Zulip):

@Wesley Wiser I've talked with @oli about this briefly but do you have a list of things to do published? otherwise could be nice if we can write something down

Wesley Wiser (Dec 22 2019 at 02:24, on Zulip):

The list is currently just

oli (Dec 22 2019 at 02:29, on Zulip):

I'm also thinking about mir librarification https://github.com/rust-lang/compiler-team/issues/233

bjorn3 (Dec 22 2019 at 15:31, on Zulip):

Found another bug (probably in the mir inliner): https://github.com/rust-lang/rust/issues/67529

oli (Dec 22 2019 at 22:33, on Zulip):

Another fun idea: https://github.com/rust-lang/rust/issues/67539#issuecomment-568308568

oli (Dec 27 2019 at 09:01, on Zulip):

I'll be there in a sec

Santiago Pastorino (Dec 27 2019 at 09:05, on Zulip):

just joined

oli (Dec 27 2019 at 09:39, on Zulip):

who is player 3? :D

Matthew Jasper (Dec 27 2019 at 09:39, on Zulip):

me

oli (Dec 27 2019 at 09:39, on Zulip):

ah :D :wave:

Matthew Jasper (Dec 27 2019 at 09:39, on Zulip):

:wave:

oli (Dec 27 2019 at 09:43, on Zulip):

ok, my notifications have been used up, my reviews are done. Ready to do stuff

vertexclique (Dec 27 2019 at 09:50, on Zulip):

visiting, I will listen carefully. :rocket:

oli (Dec 27 2019 at 09:50, on Zulip):

:D not sure how much talking will be going on

vertexclique (Dec 27 2019 at 09:51, on Zulip):

:D let's see, I will try my best.

oli (Dec 27 2019 at 09:52, on Zulip):

so the current project status is

oli (Dec 27 2019 at 09:52, on Zulip):

I'm gonna create a hackmd so we can collect all this info and update it and also add project ideas

Santiago Pastorino (Dec 27 2019 at 09:54, on Zulip):

is wesley around already?

vertexclique (Dec 27 2019 at 09:57, on Zulip):

mahmut wants to work on this #49206

oli (Dec 27 2019 at 09:59, on Zulip):

https://hackmd.io/@oli-obk/ByKo48XJU

oli (Dec 27 2019 at 10:01, on Zulip):

@mahmut related info: https://github.com/rust-lang/const-eval/pull/33#discussion_r361555939

oli (Dec 27 2019 at 10:03, on Zulip):

I think we could start with creating a lint that triggers on final constants whose value contains a non-public !Sync type

oli (Dec 27 2019 at 10:04, on Zulip):

so *const u8 is ok, so is any value of struct Foo { pub bar: *const u8 }, but not struct Bar { bar: *const u8 } and struct Mep { foo: Foo }

oli (Dec 27 2019 at 10:04, on Zulip):

basically if the value is publicly reachable, noone can make any assumptions about the fields anyway, becaues anyone can modify the fields

vertexclique (Dec 27 2019 at 10:07, on Zulip):

oh I think I got it.

bjorn3 (Dec 27 2019 at 10:07, on Zulip):
- Fix the inliner (aka resolve #63802)

I found some more mir inliner issues: https://github.com/rust-lang/rust/issues/created_by/bjorn3 (until #67529) Most of which are miscompilation and creating broken MIR.

oli (Dec 27 2019 at 10:07, on Zulip):

e.g. I see no reason why we should be forbidding static FOO: *const u8 = &1; today

oli (Dec 27 2019 at 10:08, on Zulip):

@bjorn3 are these related to the resolve problem or independent?

bjorn3 (Dec 27 2019 at 10:09, on Zulip):

The Vec::index one may be related, but the rest isn't I think.

vertexclique (Dec 27 2019 at 10:10, on Zulip):

Can I ask completely unrelated question, because I want to know the reasoning to ban/unban this in the future:
https://github.com/rust-lang/rust/issues/67191#issuecomment-568024316

DPC (Dec 27 2019 at 10:10, on Zulip):

Any small/easy issue to start with? :P

vertexclique (Dec 27 2019 at 10:11, on Zulip):

From my point of view const FOO: ! = panic!(); this is valid in no_std context.

oli (Dec 27 2019 at 10:13, on Zulip):

@vertexclique the problem isn't the constant. If you declare that constant that is completely sound if we just lint and don't error

oli (Dec 27 2019 at 10:14, on Zulip):

the problem is let x: ! = FOO; because now you have a value of type ! and if that happens at runtime that's instant UB

vertexclique (Dec 27 2019 at 10:16, on Zulip):

_enlightened_

Wesley Wiser (Dec 27 2019 at 10:21, on Zulip):

:wave: Good morning!

oli (Dec 27 2019 at 10:25, on Zulip):

:wave:

Wesley Wiser (Dec 27 2019 at 10:28, on Zulip):

I'm still plugging away at the resolve+specialization issue. @oli I rebased on top of your pr (#67631) and while I believe that did fix a few things, I'm still seeing that weird LLVM issue.

oli (Dec 27 2019 at 10:29, on Zulip):

@Wesley Wiser the resolve fix PR should actually undo #67631 and not keep using it

oli (Dec 27 2019 at 10:29, on Zulip):

I think?

Wesley Wiser (Dec 27 2019 at 10:29, on Zulip):

Well, I think after my change, there will be more things that do not resolve to a specific instance

Wesley Wiser (Dec 27 2019 at 10:30, on Zulip):

Which currently are albeit incorrectly

oli (Dec 27 2019 at 10:33, on Zulip):

but that's ok, if resolve fails with TooGeneric, const prop bails out

Wesley Wiser (Dec 27 2019 at 10:35, on Zulip):

Ah, yeah you're right

oli (Dec 27 2019 at 10:35, on Zulip):

if there's a difference whether you rebased over my PR or not, that means the resolve PR still has bugs

Wesley Wiser (Dec 27 2019 at 10:36, on Zulip):

I'm still seeing some failing ui tests so that's very possible

oli (Dec 27 2019 at 10:37, on Zulip):

do you have the current status somewhere?

oli (Dec 27 2019 at 10:37, on Zulip):

also: did you ever see any ICEs where stuff couldn't be loaded from metadata?

Wesley Wiser (Dec 27 2019 at 10:39, on Zulip):

I'll force push to my branch in a second

Wesley Wiser (Dec 27 2019 at 10:39, on Zulip):

I don't think I've ever seen that

oli (Dec 27 2019 at 10:39, on Zulip):

k good to know, because my dedup PR definitely has this problem XD

Wesley Wiser (Dec 27 2019 at 10:40, on Zulip):

But I was focused on getting the ui and mir-opt tests passing so I probably wasn't exercising the incremental functionality

oli (Dec 27 2019 at 10:40, on Zulip):

nah, this was in ui tests

Wesley Wiser (Dec 27 2019 at 10:40, on Zulip):

Oh

Wesley Wiser (Dec 27 2019 at 10:40, on Zulip):

Nah I didn't see that

oli (Dec 27 2019 at 10:40, on Zulip):

cool, that cuts down the places I need to look at

oli (Dec 27 2019 at 10:41, on Zulip):

Maybe I should wait until your PR is done and rebase over it so I have fewer things to worry about

oli (Dec 27 2019 at 10:41, on Zulip):

that deduplication is triggering many assertions in rustc that are made on assumptions that just aren't universally true

Wesley Wiser (Dec 27 2019 at 10:43, on Zulip):

https://github.com/rust-lang/rust/compare/master...wesleywiser:optimizations_handle_specialization?expand=1

Wesley Wiser (Dec 27 2019 at 10:45, on Zulip):

Here's where we're at:

Wesley Wiser (Dec 27 2019 at 10:46, on Zulip):

All other ui and mir-opt tests pass

oli (Dec 27 2019 at 10:48, on Zulip):

yea, so the "erroneous constant used" is weird. The constant being used is entriely monomorphic, but const prop still bails out on it and then codegen catches it

oli (Dec 27 2019 at 10:49, on Zulip):

Maybe it's silenced due to the reveal mode being UserFacing and not All

oli (Dec 27 2019 at 10:50, on Zulip):

oh, also you need to rebase again to get my full PR

oli (Dec 27 2019 at 10:50, on Zulip):

but that shouldn't change anything

Wesley Wiser (Dec 27 2019 at 10:51, on Zulip):

Yeah, I figured it probably wouldn't make a difference so I haven't rebased yet

oli (Dec 27 2019 at 10:52, on Zulip):

so... maybe const prop optimizing functions that are not generic should set the reveal mode to Reveal::All?

Wesley Wiser (Dec 27 2019 at 10:53, on Zulip):

I have a cut down version of ufcs-polymorphic-paths.rs that might be helpful:

// run-pass

trait Size: Sized {
    fn size() -> usize { std::mem::size_of::<Self>() }
}
impl<T> Size for T {}

pub fn main() {
    static S: fn() -> usize = bool::size;
}
Wesley Wiser (Dec 27 2019 at 10:55, on Zulip):

IIRC the fn size() function's defaultness is true

Wesley Wiser (Dec 27 2019 at 10:55, on Zulip):

So const prop should get a TooGeneric error

oli (Dec 27 2019 at 10:56, on Zulip):

huh, how is const prop active there at all?

oli (Dec 27 2019 at 10:56, on Zulip):

does it run on the static?

oli (Dec 27 2019 at 10:56, on Zulip):

do we const prop statics' and constants' bodies? :D

Wesley Wiser (Dec 27 2019 at 10:56, on Zulip):

uh

Wesley Wiser (Dec 27 2019 at 10:56, on Zulip):

Maybe I removed too much

Wesley Wiser (Dec 27 2019 at 10:57, on Zulip):

I was mostly responding to what you said here

The constant being used is entriely monomorphic, but const prop still bails out on it and then codegen catches it

oli (Dec 27 2019 at 10:58, on Zulip):

ah yes, so I think my idea would work here. Since main is monomorphic, use Reveal::all() for the promoted

Wesley Wiser (Dec 27 2019 at 11:01, on Zulip):

Won't that cause the same original issue if you have a downstream specialization of Size?

Wesley Wiser (Dec 27 2019 at 11:02, on Zulip):

Is that still appropriate for monomorphic functions because specialization doesn't require substs to be applicable.

Wesley Wiser (Dec 27 2019 at 11:03, on Zulip):

Like in the test, you could have just written Foo::my_func() and downstream there's a specialization for Foo.

Wesley Wiser (Dec 27 2019 at 11:04, on Zulip):

But your function is still monomorphic

oli (Dec 27 2019 at 11:04, on Zulip):

you can't override Size::size for any type downstream

oli (Dec 27 2019 at 11:05, on Zulip):

a) because you have impl<T> Size for T {} here without any default keyowrd

oli (Dec 27 2019 at 11:05, on Zulip):

b) because if you know all the concrete types in this crate, it can't possibly be overriden in a downstream crate because noone can implement Size for types from your crate

Wesley Wiser (Dec 27 2019 at 11:07, on Zulip):

a) because you have impl<T> Size for T {} here without any default keyowrd

Maybe this is part of the issue. From what I can tell

impl<T> Size for T {
  default fn size() -> { ... }
}

and

trait Size: Sized {
  fn size() -> { ... }
}
impl<T> Size for T { }

are represented the same in terms of AssociatedItem::defaultness

oli (Dec 27 2019 at 11:08, on Zulip):

yes and that's allright

Santiago Pastorino (Dec 27 2019 at 11:08, on Zulip):
   - rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::step                                                                                                                                                ▒
      - 48.74% rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::terminator (inlined)                                                                                                                      ▒
           rustc_mir::interpret::terminator::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_terminator (inlined)                                                                                                               ▒
         - rustc_mir::interpret::terminator::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_fn_call                                                                                                                            ▒
            - 39.91% <rustc_mir::const_eval::CompileTimeInterpreter as rustc_mir::interpret::machine::Machine>::find_mir_or_eval_fn                                                                                                          ▒
               - rustc_mir::interpret::terminator::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_const_fn_call                                                                                                                ▒
                  - 39.52% rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::copy_op (inlined)                                                                                                            ▒
                     - rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::copy_op_no_validate                                                                                                              ▒
                        - 39.28% rustc_mir::interpret::memory::Memory<M>::copy (inlined)                                                                                                                                                     ▒
                           - rustc_mir::interpret::memory::Memory<M>::copy_repeatedly                                                                                                                                                        ▒
                              - 37.58% rustc_mir::interpret::memory::Memory<M>::copy_undef_mask (inlined)                                                                                                                                    ▒
                                 - 37.02% rustc::mir::interpret::allocation::Allocation<Tag,Extra>::compress_undef_range                                                                                                                     ▒
                                    - 13.20% core::iter::range::<impl core::iter::traits::iterator::Iterator for core::ops::range::Range<A>>::next                                                                                           ▒
                                       - 8.74% core::mem::swap (inlined)                                                                                                                                                                     ▒
                                          - core::ptr::swap_nonoverlapping_one (inlined)                                                                                                                                                     ▒
                                               4.06% core::ptr::write (inlined)                                                                                                                                                              ▒
                                             - 2.95% core::ptr::read (inlined)                                                                                                                                                               ▒
                                                  core::intrinsics::copy_nonoverlapping (inlined)                                                                                                                                            ▒
                                                  core::intrinsics::overlaps (inlined)                                                                                                                                                       ▒
                                             - 1.73% core::intrinsics::copy_nonoverlapping (inlined)                                                                                                                                         ▒
                                                  core::intrinsics::overlaps (inlined)                                                                                                                                                       ▒
                                         1.48% core::cmp::impls::<impl core::cmp::PartialOrd for u64>::lt (inlined)
oli (Dec 27 2019 at 11:09, on Zulip):

but the impl<T> Size for T {} prevents any further specializations of Size

Santiago Pastorino (Dec 27 2019 at 11:09, on Zulip):

this is what's hot in my PR that wasn't before

Santiago Pastorino (Dec 27 2019 at 11:09, on Zulip):

compress_undef_range shows very high and wasn't like that before

oli (Dec 27 2019 at 11:10, on Zulip):

O_o

oli (Dec 27 2019 at 11:11, on Zulip):

which test is that happening on again?

Wesley Wiser (Dec 27 2019 at 11:11, on Zulip):

Well, that's not quite what we want though in terms of Instance::resolve() though right?

I would think we'd want the specialization version to give None when Reveal::UserFacing and the non-specialization version to give Some(...). Am I thinking about that correctly?

oli (Dec 27 2019 at 11:11, on Zulip):

let me parse this sentence XD

Wesley Wiser (Dec 27 2019 at 11:11, on Zulip):

sorry :)

Santiago Pastorino (Dec 27 2019 at 11:12, on Zulip):

which test is that happening on again?

the test is the ctfe one

oli (Dec 27 2019 at 11:12, on Zulip):

it's not your writing. it's if X None, if !X Some

Santiago Pastorino (Dec 27 2019 at 11:12, on Zulip):

which test is that happening on again?

the test is the ctfe one

particularly type LargeUninit = MaybeUninit<[u8; 1 << 24]>;

Wesley Wiser (Dec 27 2019 at 11:13, on Zulip):

Is this another instance of the "trying to eval too large a Place" issue?

oli (Dec 27 2019 at 11:13, on Zulip):

yes-ish

Wesley Wiser (Dec 27 2019 at 11:14, on Zulip):

(#67539 is the issue I'm thinking of)

Santiago Pastorino (Dec 27 2019 at 11:14, on Zulip):

why is copying being executed for uninitialized memory

Santiago Pastorino (Dec 27 2019 at 11:15, on Zulip):

unsure what the code does

Santiago Pastorino (Dec 27 2019 at 11:15, on Zulip):

but seems like it should return earlier?

oli (Dec 27 2019 at 11:15, on Zulip):

yes it should :D

Santiago Pastorino (Dec 27 2019 at 11:16, on Zulip):

ok gonna try to see why is this not happening :)

oli (Dec 27 2019 at 11:17, on Zulip):

because it's not implemented

Santiago Pastorino (Dec 27 2019 at 11:17, on Zulip):

ohh, for which case? hold on :)

oli (Dec 27 2019 at 11:17, on Zulip):

wesley found the correct issue

Santiago Pastorino (Dec 27 2019 at 11:18, on Zulip):

ahh this #67539 is the problem I'm hitting?

Wesley Wiser (Dec 27 2019 at 11:30, on Zulip):

Well, that's not quite what we want though in terms of Instance::resolve() though right?

I would think we'd want the specialization version to give None when Reveal::UserFacing and the non-specialization version to give Some(...). Am I thinking about that correctly?

I think you've got it but to reformulate,

When Reveal::UserFacing, I'd expect that:

Wesley Wiser (Dec 27 2019 at 11:30, on Zulip):

Unless

Wesley Wiser (Dec 27 2019 at 11:32, on Zulip):

Is this allowed?

trait Foo {
  fn bar() -> u8 { 42 }
}


struct X;
trait Baz { }

impl Baz for X { }

impl<T: Baz> Foo for T {
  default fn bar() -> u8 { 1 }
}

impl Foo for X {
  fn bar() -> u8 { 2 }
}
Wesley Wiser (Dec 27 2019 at 11:32, on Zulip):

I don't remember seeing this in the rfc

oli (Dec 27 2019 at 11:36, on Zulip):

hm... that may be possible

oli (Dec 27 2019 at 11:37, on Zulip):

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=056cb3d6b5708310bb7eabc141922f7b

oli (Dec 27 2019 at 11:37, on Zulip):

jup that works

Wesley Wiser (Dec 27 2019 at 11:38, on Zulip):

So I guess we can't do this

Trait default methods cause Some(...) to be returned

oli (Dec 27 2019 at 11:38, on Zulip):

no

Wesley Wiser (Dec 27 2019 at 11:38, on Zulip):

So differentiating those cases doesn't matter

Wesley Wiser (Dec 27 2019 at 11:41, on Zulip):

So then, per your idea, we can pass Reveal::All when evaluating promoted & constant values in a monomorphic function because either 1) we declared the type in which case we have all the relevant impls and coherence prevents downstream crates from adding new ones 2) this is somebody else's type and we can see all their impls.

oli (Dec 27 2019 at 11:42, on Zulip):

well or 3) we can't possibly be using a type from a downstream crate XD

oli (Dec 27 2019 at 11:43, on Zulip):

I need a troll emoticon

oli (Dec 27 2019 at 11:43, on Zulip):

although... this is basically the monomorphic function rule you wrote

Wesley Wiser (Dec 27 2019 at 11:44, on Zulip):

coherence prevents downstream crates from adding new ones

Is this actually true? Doesn't #[fundamental] let you do some horrible things?

Wesley Wiser (Dec 27 2019 at 11:45, on Zulip):

/me doesn't actually know what #[fundamental] does

oli (Dec 27 2019 at 11:45, on Zulip):

oh god

Wesley Wiser (Dec 27 2019 at 11:45, on Zulip):

I take it that's not a good sign :laughter_tears:

oli (Dec 27 2019 at 11:46, on Zulip):

:thinking:

oli (Dec 27 2019 at 11:47, on Zulip):

I don't think you can break coherence even with fundamental types

Wesley Wiser (Dec 27 2019 at 11:47, on Zulip):

That's a relief

oli (Dec 27 2019 at 11:47, on Zulip):

though full specialization may be unsound together with fundamental types, but I'll leave that to the experts

oli (Dec 27 2019 at 11:48, on Zulip):

for now, let's keep the simple case working as it was working before :slight_smile:

oli (Dec 27 2019 at 11:48, on Zulip):

oh I broke miri

oli (Dec 27 2019 at 11:48, on Zulip):

/me goes fix stuff

Santiago Pastorino (Dec 27 2019 at 12:10, on Zulip):

@oli https://github.com/rust-lang/rust/pull/67658

oli (Dec 27 2019 at 12:10, on Zulip):

wifi died, am back now

Santiago Pastorino (Dec 27 2019 at 12:10, on Zulip):

after that I hope the main PR is ok :)

oli (Dec 27 2019 at 12:12, on Zulip):

let's run perf I guess? Or you test it for the heavily regressed file manually?

Santiago Pastorino (Dec 27 2019 at 12:13, on Zulip):

yep, I can do all that again :)

Santiago Pastorino (Dec 27 2019 at 12:14, on Zulip):

I guess I'm going to rebase the whole PR on top of master, your fix is already in and I can sit the place one on top of this fix

Santiago Pastorino (Dec 27 2019 at 12:14, on Zulip):

then run perf and see what happens

oli (Dec 27 2019 at 12:14, on Zulip):

jup

oli (Dec 27 2019 at 12:14, on Zulip):

also, early return ;)

bjorn3 (Dec 27 2019 at 12:14, on Zulip):

Just suggested that too :)

oli (Dec 27 2019 at 12:15, on Zulip):

XD

Santiago Pastorino (Dec 27 2019 at 12:15, on Zulip):

ok if both of you have strong preference on that :+1:

Santiago Pastorino (Dec 27 2019 at 12:16, on Zulip):

I kind of preferred to hit the existing Ok but gonna change to the early return version

oli (Dec 27 2019 at 12:18, on Zulip):

most of the functions in that file use early return

oli (Dec 27 2019 at 12:18, on Zulip):

just look two functions up. early return even though the body is trivial afterwards

Santiago Pastorino (Dec 27 2019 at 12:23, on Zulip):

yeah already fixed it :)

Wesley Wiser (Dec 27 2019 at 12:35, on Zulip):

Are you sure that fixes the regression in #67539? When I tested the repro, I was still seeing slowness (see https://github.com/rust-lang/rust/issues/67539#issuecomment-568627324)

Wesley Wiser (Dec 27 2019 at 12:36, on Zulip):

I don't think we should close that issue until we're sure it's resolved.

Wesley Wiser (Dec 27 2019 at 12:36, on Zulip):

@Santiago Pastorino ^

Santiago Pastorino (Dec 27 2019 at 12:37, on Zulip):

yeah I'm not sure, I need to run perf

Santiago Pastorino (Dec 27 2019 at 12:37, on Zulip):

I guess I should've marked the PR as draft

Wesley Wiser (Dec 27 2019 at 12:38, on Zulip):

It's fine :)

Santiago Pastorino (Dec 27 2019 at 12:38, on Zulip):

I need to rebase the other PR first and place it on top of that one, wait hours to compile and then run the thing so maybe in 2hs I can answer :)

Wesley Wiser (Dec 27 2019 at 12:38, on Zulip):

I just think the issue you're hitting is related but a little different than OP's and we shouldn't close the issue until we're sure their issue is resolved too

Santiago Pastorino (Dec 27 2019 at 12:38, on Zulip):

ahh ya, that's also right yeah

Santiago Pastorino (Dec 27 2019 at 12:39, on Zulip):

my guess is that my issue is going to be fixed by this

Wesley Wiser (Dec 27 2019 at 12:48, on Zulip):

Ah hah. @oli I think we need a .with_reveal_all() here. codegen_static_initializer() calls const_eval_poly which was getting TooGeneric.

Wesley Wiser (Dec 27 2019 at 12:50, on Zulip):

Ugh but that causes another issue

error[E0391]: cycle detected when const-evaluating + checking `Alpha::V3::{{constant}}#0`
  --> /home/wesley/code/rust/rust/src/test/ui/type-alias-enum-variants/self-in-enum-definition.rs:5:10
   |
LL |     V3 = Self::V1 {} as u8 + 2, //~ ERROR cycle detected when const-evaluating
   |          ^^^^^^^^
   |
note: ...which requires const-evaluating + checking `Alpha::V3::{{constant}}#0`...
  --> /home/wesley/code/rust/rust/src/test/ui/type-alias-enum-variants/self-in-enum-definition.rs:5:10
   |
LL |     V3 = Self::V1 {} as u8 + 2, //~ ERROR cycle detected when const-evaluating
   |          ^^^^^^^^
note: ...which requires const-evaluating `Alpha::V3::{{constant}}#0`...
  --> /home/wesley/code/rust/rust/src/test/ui/type-alias-enum-variants/self-in-enum-definition.rs:5:10
   |
LL |     V3 = Self::V1 {} as u8 + 2, //~ ERROR cycle detected when const-evaluating
   |          ^^^^^^^^
   = note: ...which requires computing layout of `Alpha`...
   = note: ...which again requires const-evaluating + checking `Alpha::V3::{{constant}}#0`, completing the cycle
note: cycle used when collecting item types in top-level module
  --> /home/wesley/code/rust/rust/src/test/ui/type-alias-enum-variants/self-in-enum-definition.rs:1:1
   |
LL | / #[repr(u8)]
LL | | enum Alpha {
LL | |     V1 = 41,
LL | |     V2 = Self::V1 as u8 + 1, // OK; See #50072.
...  |
LL | |
LL | | fn main() {}
   | |____________^

error: aborting due to previous error
oli (Dec 27 2019 at 13:12, on Zulip):

wait how is that an issue? the cycle existed before your PR, too

Santiago Pastorino (Dec 27 2019 at 13:32, on Zulip):

was trying out the following example ...

Santiago Pastorino (Dec 27 2019 at 13:33, on Zulip):
fn main() {
    let _ = [(); 30];
}
Santiago Pastorino (Dec 27 2019 at 13:33, on Zulip):
[santiago@galago tmp]$ rustc --version
rustc 1.41.0-nightly (c8ea4ace9 2019-12-14)
[santiago@galago tmp]$ time rustc test.rs

real    0m0.428s
user    0m0.317s
sys 0m0.041s
Santiago Pastorino (Dec 27 2019 at 13:33, on Zulip):
[santiago@galago rust2 (do-not-copy-zsts)]$ time ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs

real    0m0.774s
user    0m0.592s
sys 0m0.120s
Santiago Pastorino (Dec 27 2019 at 13:34, on Zulip):

that one is https://github.com/rust-lang/rust/pull/67658

Santiago Pastorino (Dec 27 2019 at 13:34, on Zulip):

but I guess I have an old nightly

Santiago Pastorino (Dec 27 2019 at 13:34, on Zulip):

may be good idea to just try with master

Santiago Pastorino (Dec 27 2019 at 13:35, on Zulip):

going to do that

bjorn3 (Dec 27 2019 at 13:36, on Zulip):

Also try using hyperfine for benchmarking and using a bigger array.

oli (Dec 27 2019 at 14:13, on Zulip):

hmm... all my PRs are up to date (enough)... what to hack on now?

oli (Dec 27 2019 at 14:14, on Zulip):

btw @Wesley Wiser do you still have the llvm failures?

Wesley Wiser (Dec 27 2019 at 14:14, on Zulip):

If I do this that resolves the LLVM issue

Wesley Wiser (Dec 27 2019 at 14:14, on Zulip):

But causes the cycle

oli (Dec 27 2019 at 14:15, on Zulip):

but the cycle is on master, too

Wesley Wiser (Dec 27 2019 at 14:15, on Zulip):

What do you mean?

Wesley Wiser (Dec 27 2019 at 14:15, on Zulip):

I don't think it was failing before my change

oli (Dec 27 2019 at 14:15, on Zulip):

https://github.com/rust-lang/rust/blob/41501a6b03a8f10d8c29dfcb37dbd5ff84b33f34/src/test/ui/type-alias-enum-variants/self-in-enum-definition.rs#L5

oli (Dec 27 2019 at 14:15, on Zulip):

this is master

Wesley Wiser (Dec 27 2019 at 14:16, on Zulip):

huh

oli (Dec 27 2019 at 14:16, on Zulip):

:D

Wesley Wiser (Dec 27 2019 at 14:16, on Zulip):

Let me retest

Wesley Wiser (Dec 27 2019 at 14:16, on Zulip):

I mean the test isn't passing

oli (Dec 27 2019 at 14:16, on Zulip):

sure, maybe you need to --bless

oli (Dec 27 2019 at 14:16, on Zulip):

or maybe the span changed?

Wesley Wiser (Dec 27 2019 at 14:16, on Zulip):

Oh that could be

Wesley Wiser (Dec 27 2019 at 14:17, on Zulip):

(Building....)

oli (Dec 27 2019 at 14:17, on Zulip):

like you probably have one less element in the query stack

oli (Dec 27 2019 at 14:17, on Zulip):

eh more

Wesley Wiser (Dec 27 2019 at 14:18, on Zulip):

We still have the change to the miri unleashed tests

oli (Dec 27 2019 at 14:18, on Zulip):

because now you enter the const_eval query with reveal::all, which still tries reveal::UserFacing first

Wesley Wiser (Dec 27 2019 at 14:18, on Zulip):

Are you concerned about that?

oli (Dec 27 2019 at 14:18, on Zulip):

let me check the other thread, sec

oli (Dec 27 2019 at 14:19, on Zulip):

ah

oli (Dec 27 2019 at 14:19, on Zulip):

wait that wasn't fixed by this?

Wesley Wiser (Dec 27 2019 at 14:19, on Zulip):

I can double check but no, I believe it's still changed

oli (Dec 27 2019 at 14:19, on Zulip):

ah

oli (Dec 27 2019 at 14:19, on Zulip):

right

oli (Dec 27 2019 at 14:19, on Zulip):

these are constants

oli (Dec 27 2019 at 14:20, on Zulip):

you fixed statics

Wesley Wiser (Dec 27 2019 at 14:21, on Zulip):

You said earlier about eval-ing constants with Reveal::All, I assume that would fix that

Wesley Wiser (Dec 27 2019 at 14:21, on Zulip):

So I should probably do that

oli (Dec 27 2019 at 14:21, on Zulip):

basically https://github.com/rust-lang/rust/blob/41501a6b03a8f10d8c29dfcb37dbd5ff84b33f34/src/librustc_mir/transform/const_prop.rs#L296 needs the same change if https://github.com/rust-lang/rust/blob/41501a6b03a8f10d8c29dfcb37dbd5ff84b33f34/src/librustc_mir/transform/const_prop.rs#L301 is !needs_subst()

Wesley Wiser (Dec 27 2019 at 14:22, on Zulip):

Easy enough

Wesley Wiser (Dec 27 2019 at 14:22, on Zulip):

I should have test results in two or three minutes

oli (Dec 27 2019 at 14:22, on Zulip):

:+1:

oli (Dec 27 2019 at 14:22, on Zulip):

I need to figure out some day what tcx.param_env actually does

Wesley Wiser (Dec 27 2019 at 14:23, on Zulip):

:lol:

Wesley Wiser (Dec 27 2019 at 14:26, on Zulip):

You were right

Wesley Wiser (Dec 27 2019 at 14:26, on Zulip):
diff --git a/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr b/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr
index dc4050e44ab..db535b53fcf 100644
--- a/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr
+++ b/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr
@@ -4,6 +4,11 @@ error[E0391]: cycle detected when const-evaluating + checking `Alpha::V3::{{cons
 LL |     V3 = Self::V1 {} as u8 + 2,
    |          ^^^^^^^^
    |
+note: ...which requires const-evaluating + checking `Alpha::V3::{{constant}}#0`...
+  --> $DIR/self-in-enum-definition.rs:5:10
+   |
+LL |     V3 = Self::V1 {} as u8 + 2,
+   |          ^^^^^^^^
 note: ...which requires const-evaluating `Alpha::V3::{{constant}}#0`...
   --> $DIR/self-in-enum-definition.rs:5:10
    |
bjorn3 (Dec 27 2019 at 14:27, on Zulip):

I need to figure out some day what tcx.param_env actually does

I think the docs for it are a good starter.

The docs for ParamEnv:

When type checking, we use the ParamEnv to track details about the set of where-clauses that are in scope at this particular point.

The docs for Reveal:

Depending on the stage of compilation, we want projection to be more or less conservative.

At type-checking time, we refuse to project any associated type that is marked default.

At codegen time, all monomorphic projections will succeed. Also, impl Trait is normalized to the concrete type, which has to be already collected by type-checking.

Wesley Wiser (Dec 27 2019 at 14:28, on Zulip):

So there's one remaining failure:

---- [ui] ui/rfcs/rfc-2005-default-binding-mode/constref.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 1
command: "/home/wesley/code/rust/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/home/wesley/code/rust/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/home/wesley/code/rust/rust/src/test/ui/rfcs/rfc-2005-default-binding-mode/constref.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/home/wesley/code/rust/rust/build/x86_64-unknown-linux-gnu/test/ui/rfcs/rfc-2005-default-binding-mode/constref/a" "-Crpath" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/home/wesley/code/rust/rust/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/home/wesley/code/rust/rust/build/x86_64-unknown-linux-gnu/test/ui/rfcs/rfc-2005-default-binding-mode/constref/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error[E0158]: associated consts cannot be referenced in patterns
  --> /home/wesley/code/rust/rust/src/test/ui/rfcs/rfc-2005-default-binding-mode/constref.rs:32:10
   |
LL |         (i32::CONST_REF_DEFAULT, i32::CONST_REF, i64::CONST_REF_DEFAULT, i64::CONST_REF) => true,
   |          ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0158`.

------------------------------------------



failures:
    [ui] ui/rfcs/rfc-2005-default-binding-mode/constref.rs
Wesley Wiser (Dec 27 2019 at 14:30, on Zulip):

basically https://github.com/rust-lang/rust/blob/41501a6b03a8f10d8c29dfcb37dbd5ff84b33f34/src/librustc_mir/transform/const_prop.rs#L296 needs the same change if https://github.com/rust-lang/rust/blob/41501a6b03a8f10d8c29dfcb37dbd5ff84b33f34/src/librustc_mir/transform/const_prop.rs#L301 is !needs_subst()

Actually, wait. Won't this cause us to use Reveal::All for everything not just constants?

The same logic we discussed earlier applies to everything not just constants.

oli (Dec 27 2019 at 14:30, on Zulip):

yes, but if the thing is monomorphic, shouldn't we?

Wesley Wiser (Dec 27 2019 at 14:31, on Zulip):

Yeah

oli (Dec 27 2019 at 14:32, on Zulip):

so the remaining failure is super weird

oli (Dec 27 2019 at 14:32, on Zulip):

that test checks whether associated constants can be used in patterns!?

oli (Dec 27 2019 at 14:32, on Zulip):

so that feature appears to exists

Wesley Wiser (Dec 27 2019 at 14:32, on Zulip):

Yeah

oli (Dec 27 2019 at 14:32, on Zulip):

what on earth is that error

Wesley Wiser (Dec 27 2019 at 14:33, on Zulip):

I remember spending a bunch of time looking at this Christmas Eve but I don't remember what I found

Wesley Wiser (Dec 27 2019 at 14:33, on Zulip):

I think I came to the conclusion something in the HIR -> MIR layer needed to use Reveal::All but that caused a bunch of other issues

oli (Dec 27 2019 at 14:33, on Zulip):

@bjorn3 I understand what ParamEnv does, I don't understand how tcx.param_env can create a ParamEnv from the ether. Like what where clauses are available here?

Wesley Wiser (Dec 27 2019 at 14:34, on Zulip):

And seems like a bad fix to me

oli (Dec 27 2019 at 14:34, on Zulip):

I am mostly wondering why the rustc source contains "associated consts cannot be referenced in patterns" somewhere if we have a feature permitting just that

Wesley Wiser (Dec 27 2019 at 14:34, on Zulip):

huh

Wesley Wiser (Dec 27 2019 at 14:34, on Zulip):

It's not feature gated?

oli (Dec 27 2019 at 14:35, on Zulip):

nope: no gates in this test: https://github.com/rust-lang/rust/blob/master/src/test/ui/rfcs/rfc-2005-default-binding-mode/constref.rs

Matthew Jasper (Dec 27 2019 at 14:35, on Zulip):

bjorn3 I understand what ParamEnv does, I don't understand how tcx.param_env can create a ParamEnv from the ether. Like what where clauses are available here?

It takes a DefId, it gets them from there (using predicates_of)

oli (Dec 27 2019 at 14:35, on Zulip):

I think what the error message is trying to tell us is that you can't write Trait::ASSOC_CONSTANT without this somehow resolving to a ConcreteType::ASSOC_CONSTANT

oli (Dec 27 2019 at 14:37, on Zulip):

@Matthew Jasper oh, so it's basically just like IdentitySubsts, you get all the where bounds, but they are still polymorphic where bounds

Wesley Wiser (Dec 27 2019 at 14:37, on Zulip):

Yeah seems like a bad error message

Matthew Jasper (Dec 27 2019 at 14:37, on Zulip):

Yes

Wesley Wiser (Dec 27 2019 at 14:38, on Zulip):

Maybe it needs to say something like "generic associated constants"

oli (Dec 27 2019 at 14:40, on Zulip):

@Wesley Wiser ok so I think I know what's going on. This is supposed to catch T::ASSOC_CONST in patterns, but it currently does so by relying on const eval to bail out with TooGeneric. Your changes now make it bail out immediately for ConcreteType::TRAIT_ASSOC_CONST because that is only available in reveal_all mode

Wesley Wiser (Dec 27 2019 at 14:40, on Zulip):

Interesting...

Wesley Wiser (Dec 27 2019 at 14:40, on Zulip):

(The const prop change fixes those miri unleashed constant tests BTW)

oli (Dec 27 2019 at 14:41, on Zulip):

this seems totally fine to use self.param_env.with_reveal_all(), because just like with const prop of non-generic functions, patterns are always monomorphic even if their function isn't

oli (Dec 27 2019 at 14:41, on Zulip):

now you said something about more failures being caused by this

oli (Dec 27 2019 at 14:42, on Zulip):

so I'm unsure what's up with that, do you remember anything?

Wesley Wiser (Dec 27 2019 at 14:42, on Zulip):

I may have made the change too high in the compilation process so it affected too many things

oli (Dec 27 2019 at 14:42, on Zulip):

oh also please leave comments at all the with_reveal_all sites explaining why we are using it for each specific site

Wesley Wiser (Dec 27 2019 at 14:42, on Zulip):

Let see if I can pull it out of the reflog

oli (Dec 27 2019 at 14:43, on Zulip):

yea if you made the param_env field reveal_all I'd totally understand this causing more failures

Wesley Wiser (Dec 27 2019 at 14:44, on Zulip):

I think this was the patch I tried:

diff --git a/src/librustc_mir/hair/pattern/mod.rs b/src/librustc_mir/hair/pattern/mod.rs
index 869aeeba418..051a0177022 100644
--- a/src/librustc_mir/hair/pattern/mod.rs
+++ b/src/librustc_mir/hair/pattern/mod.rs
@@ -743,7 +743,7 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> {
         let kind = match res {
             Res::Def(DefKind::Const, def_id) | Res::Def(DefKind::AssocConst, def_id) => {
                 let substs = self.tables.node_substs(id);
-                match self.tcx.const_eval_resolve(self.param_env, def_id, substs, Some(span)) {
+                match self.tcx.const_eval_resolve(self.param_env.with_reveal_all(), def_id, substs, Some(span)) {
                     Ok(value) => {
                         let pattern = self.const_to_pat(value, id, span);
                         if !is_associated_const {
oli (Dec 27 2019 at 14:45, on Zulip):

yea, that looks correct to me

Wesley Wiser (Dec 27 2019 at 14:45, on Zulip):

Ok. Let me try applying it and see what happens.

Wesley Wiser (Dec 27 2019 at 14:50, on Zulip):
test result: ok. 9410 passed; 0 failed; 45 ignored; 0 measured; 0 filtered out

:tada:

oli (Dec 27 2019 at 14:51, on Zulip):

woooo, awesome

oli (Dec 27 2019 at 14:51, on Zulip):

:ship: it :D (ok no, I'll review first)

Wesley Wiser (Dec 27 2019 at 14:51, on Zulip):

Writing comments now

Wesley Wiser (Dec 27 2019 at 14:51, on Zulip):

:)

oli (Dec 27 2019 at 14:54, on Zulip):

in case the rollup in https://github.com/rust-lang/rust/pull/67660 goes through, can you also remove my hacks from https://github.com/rust-lang/rust/pull/67631 in your PR?

Wesley Wiser (Dec 27 2019 at 14:54, on Zulip):

Yeah I can do that

Santiago Pastorino (Dec 27 2019 at 15:03, on Zulip):
[santiago@galago tmp]$ hyperfine --warmup 3 'rustc /tmp/test.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs'
Benchmark #1: rustc /tmp/test.rs
  Time (mean ± σ):      2.977 s ±  0.124 s    [User: 2.641 s, System: 0.092 s]
  Range (min … max):    2.761 s …  3.140 s    10 runs

Benchmark #2: ~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs
  Time (mean ± σ):      4.334 s ±  0.240 s    [User: 3.888 s, System: 0.131 s]
  Range (min … max):    3.923 s …  4.642 s    10 runs

Summary
  'rustc /tmp/test.rs' ran
    1.46 ± 0.10 times faster than '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs'
[santiago@galago tmp]$ hyperfine --warmup 3 '/home/santiago/src/oss/r /rustc /tmp/test.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs'
rayon/       rust0/       rust1/       rust2/       rust3/       rustc-guide/ rust-clippy/ rustc-perf/
[santiago@galago tmp]$ hyperfine --warmup 3 '/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs'
Benchmark #1: /home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs
  Time (mean ± σ):      4.726 s ±  0.273 s    [User: 4.293 s, System: 0.172 s]
  Range (min … max):    4.045 s …  5.002 s    10 runs

Benchmark #2: ~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs
  Time (mean ± σ):      4.606 s ±  0.181 s    [User: 4.251 s, System: 0.140 s]
  Range (min … max):    4.310 s …  4.978 s    10 runs

Summary
  '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs' ran
    1.03 ± 0.07 times faster than '/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs'
Santiago Pastorino (Dec 27 2019 at 15:04, on Zulip):

rustc is rustc 1.41.0-nightly (c8ea4ace9 2019-12-14)

Santiago Pastorino (Dec 27 2019 at 15:04, on Zulip):

the one in rust1 is master and the one in rust2 is master with the patch we were talking about

Santiago Pastorino (Dec 27 2019 at 15:04, on Zulip):

so it doesn't fix the issue reported

Santiago Pastorino (Dec 27 2019 at 15:05, on Zulip):

I can try and check if it does fix my issue

Santiago Pastorino (Dec 27 2019 at 15:11, on Zulip):

and my test ...

Santiago Pastorino (Dec 27 2019 at 15:11, on Zulip):
[santiago@galago myapp (master)]$ hyperfine --warmup 3 '/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs'
Benchmark #1: /home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs
  Time (mean ± σ):      7.364 s ±  0.140 s    [User: 6.524 s, System: 0.707 s]
  Range (min … max):    7.101 s …  7.585 s    10 runs

Benchmark #2: ~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs
  Time (mean ± σ):      6.539 s ±  0.265 s    [User: 5.828 s, System: 0.523 s]
  Range (min … max):    6.291 s …  7.071 s    10 runs

Summary
  '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs' ran
    1.13 ± 0.05 times faster than '/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs'
[santiago@galago myapp (master)]$ hyperfine --warmup 3 '/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs'
Santiago Pastorino (Dec 27 2019 at 15:11, on Zulip):

makes it a bit faster

oli (Dec 27 2019 at 15:13, on Zulip):

hmm... so I looked at your callgraph dump

Santiago Pastorino (Dec 27 2019 at 15:13, on Zulip):
[santiago@galago myapp (master)]$ hyperfine --warmup 3 'rustc /tmp/test.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs'
Benchmark #1: rustc /tmp/test.rs
  Time (mean ± σ):      1.438 s ±  0.154 s    [User: 1.376 s, System: 0.048 s]
  Range (min … max):    1.310 s …  1.820 s    10 runs

Benchmark #2: ~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs
  Time (mean ± σ):      1.840 s ±  0.336 s    [User: 1.763 s, System: 0.057 s]
  Range (min … max):    1.576 s …  2.555 s    10 runs

Summary
  'rustc /tmp/test.rs' ran
    1.28 ± 0.27 times faster than '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /tmp/test.rs'
oli (Dec 27 2019 at 15:13, on Zulip):

it looks like you're not on top of master

Santiago Pastorino (Dec 27 2019 at 15:13, on Zulip):

yeah the callgraph dump is old

oli (Dec 27 2019 at 15:13, on Zulip):

ok

oli (Dec 27 2019 at 15:13, on Zulip):

So we are in https://github.com/rust-lang/rust/blob/3ac40b69c75929dac5115b6a49eb4f1ecc352416/src/librustc_mir/const_eval/machine.rs#L25

Santiago Pastorino (Dec 27 2019 at 15:14, on Zulip):

so this patch makes things a bit faster 3% for the reported issue and 13% for my case

Santiago Pastorino (Dec 27 2019 at 15:14, on Zulip):

still my case is 27% slower than an old nightly

oli (Dec 27 2019 at 15:14, on Zulip):

https://github.com/rust-lang/rust/blob/3ac40b69c75929dac5115b6a49eb4f1ecc352416/src/librustc_mir/const_eval/machine.rs#L53 seems to be the culprit entry point

Santiago Pastorino (Dec 27 2019 at 15:14, on Zulip):

yes

Santiago Pastorino (Dec 27 2019 at 15:15, on Zulip):

can we break out of that earlier would be the question?

oli (Dec 27 2019 at 15:15, on Zulip):

what's the program you're compiling right now to check the perf?

Santiago Pastorino (Dec 27 2019 at 15:17, on Zulip):
#![feature(const_fn, const_let)]
use std::mem::MaybeUninit;

// Try to make CTFE actually do a lot of computation, without producing a big result.
// The const fn expressions evaluated here take a dummy u32 argument because otherwise
// const fn memoisation is able to eliminate a lot of the work.
// And without support for loops.

macro_rules! const_repeat {
    // Base case: Use 16 at the end to avoid function calls at the leafs as much as possible.
    ([16] $e: expr, $T: ty) => {{
        $e; $e; $e; $e;
        $e; $e; $e; $e;
        $e; $e; $e; $e;
        $e; $e; $e; $e
    }};
    ([1] $e: expr, $T: ty) => {{
        $e
    }};
    // Recursive case: Take a 16
    ([16 $($n: tt)*] $e: expr, $T: ty) => {{
        const fn e(_: u32) -> $T { const_repeat!([$($n)*] $e, $T) }
        e(0); e(0); e(0); e(0);
        e(0); e(0); e(0); e(0);
        e(0); e(0); e(0); e(0);
        e(0); e(0); e(0); e(0)
    }};
    // Recursive case: Take a 8
    ([8 $($n: tt)*] $e: expr, $T: ty) => {{
        const fn e(_: u32) -> $T { const_repeat!([$($n)*] $e, $T) }
        e(0); e(0); e(0); e(0);
        e(0); e(0); e(0); e(0)
    }};
    // Recursive case: Take a 4
    ([4 $($n: tt)*] $e: expr, $T: ty) => {{
        const fn e(_: u32) -> $T { const_repeat!([$($n)*] $e, $T) }
        e(0); e(0); e(0); e(0)
    }};
    // Recursive case: Take a 2
    ([2 $($n: tt)*] $e: expr, $T: ty) => {{
        const fn e(_: u32) -> $T { const_repeat!([$($n)*] $e, $T) }
        e(0); e(0)
    }};
}
macro_rules! expensive_static {
    ($name: ident : $T: ty = $e : expr; $count: tt) =>
        (pub static $name : $T = const_repeat!($count $e, $T);)
}

pub trait Trait: Sync {}
impl Trait for u32 {}

const fn inc(i: i32) -> i32 { i + 1 }

// The numbers in the brackets are iteration counts. E.g., [4 16 16] means
// 4 * 16 * 16 = 2^(2+4+4) = 2^10 iterations.
//expensive_static!(CAST: usize = 42i32 as u8 as u64 as i8 as isize as usize; [8 16 16 16 16]);
//expensive_static!(CONST_FN: i32 = inc(42); [8 16 16 16 16]);
//expensive_static!(FIELDS: &'static i32 = &("bar", 42, "foo", 3.14).1; [8 16 16 16 16]);
//expensive_static!(FORCE_ALLOC: i32 = *****(&&&&&5); [8 16 16 16 16]);
//expensive_static!(CHECKED_INDEX: u8 = b"foomp"[3]; [8 16 16 16 16]);
//expensive_static!(OPS: i32 = ((((10 >> 1) + 3) * 7) / 2 - 12) << 4; [4 16 16 16 16]);
//expensive_static!(RELOCATIONS : &'static str = "hello"; [8 16 16 16 16]);
//expensive_static!(UNSIZE_SLICE: &'static [u8] = b"foo"; [4 16 16 16 16 16]);
//expensive_static!(UNSIZE_TRAIT: &'static Trait = &42u32; [4 16 16 16 16 16]);

// copying all these zeros and the corresponding definedness bits can be expensive and is probably
// prone to regressions.
// 24 is an exponent that makes the repeat expression take less than two seconds to compute
//const FOO: [i32; 1 << 30] = [0; 1 << 30];

// Try CTFE that operate on values that contain largely uninitialized memory, not requiring any
// particular representation in MIR.
type LargeUninit = MaybeUninit<[u8; 1 << 24]>;

// copying uninitialized bytes could also be expensive and could be optimized independently, so
// track regressions here separately. It should also be less costly to compose new values
// containing largly undef bytes.
const BAR: LargeUninit = MaybeUninit::uninit();

// Check the evaluation time of passing through a function.
const fn id<T>(val: T) -> T { val }
const ID: LargeUninit = id(MaybeUninit::uninit());

const fn build() -> LargeUninit { MaybeUninit::uninit() }
const BUILD: LargeUninit = build();

// Largely uninitialized memory but initialized with tag at the start, in both cases.
const NONE: Option<LargeUninit> = None;
const SOME: Option<LargeUninit> = Some(MaybeUninit::uninit());

// A large uninit surrounded by initialized bytes whose representation is surely computed.
const SURROUND: (u8, LargeUninit, u8) = (0, MaybeUninit::uninit(), 0);
const SURROUND_ID: (u8, LargeUninit, u8) = id((0, MaybeUninit::uninit(), 0));

// Check actual codegen for these values.
pub static STATIC_BAR: LargeUninit = MaybeUninit::uninit();
pub static STATIC_NONE: Option<LargeUninit> = None;
pub static STATIC_SOME: Option<LargeUninit> = Some(MaybeUninit::uninit());

fn main() {}
Santiago Pastorino (Dec 27 2019 at 15:17, on Zulip):

i guess I can reduce that and just leave the LargeUninit stuff around

Santiago Pastorino (Dec 27 2019 at 15:17, on Zulip):

I can run perf again with the updated thing too

Santiago Pastorino (Dec 27 2019 at 15:20, on Zulip):

so ...

Santiago Pastorino (Dec 27 2019 at 15:21, on Zulip):

running perf on my nightly and my pr

Santiago Pastorino (Dec 27 2019 at 15:21, on Zulip):
    45.83%             librustc_driver-fb30b83f718b75a1.so  [.] rustc::mir::interpret::allocation::Allocation<Tag,Extra>::compress_undef_range
    38.10%             librustc_driver-fb30b83f718b75a1.so  [.] core::iter::range::<impl core::iter::traits::iterator::Iterator for core::ops::range::Range<A>>::next
     7.35%             librustc_driver-fb30b83f718b75a1.so  [.] <rustc_hash::FxHasher as core::hash::Hasher>::write
Santiago Pastorino (Dec 27 2019 at 15:21, on Zulip):

this is perf diff

Santiago Pastorino (Dec 27 2019 at 15:21, on Zulip):

45% slowdown in compress_undef_range

Santiago Pastorino (Dec 27 2019 at 15:26, on Zulip):

and that's basically copy_repeatedly

Santiago Pastorino (Dec 27 2019 at 15:28, on Zulip):

ok so it seems like this is not returning early

oli (Dec 27 2019 at 15:37, on Zulip):

walking fuchur, I'll be back in an hour

Santiago Pastorino (Dec 27 2019 at 16:19, on Zulip):

@oli just in case, I didn't run perf on the issue so I'm not really sure what's going on

oli (Dec 27 2019 at 16:19, on Zulip):

Oh right, the PR you Rebased over only cares about zsts

Santiago Pastorino (Dec 27 2019 at 16:19, on Zulip):

in my case the example I'm running is not a zst

oli (Dec 27 2019 at 16:19, on Zulip):

Yea

Santiago Pastorino (Dec 27 2019 at 16:19, on Zulip):

so of course the example is not going to help

Santiago Pastorino (Dec 27 2019 at 16:20, on Zulip):

unsure what to do for my case

oli (Dec 27 2019 at 16:20, on Zulip):

There was a PR by someone. They closed it themselves b/c they don't have time. Iirc that prevented copies on undef

Wesley Wiser (Dec 27 2019 at 16:20, on Zulip):

Can we resolve this by optimizing compress_undef_range() more?

oli (Dec 27 2019 at 16:22, on Zulip):

Not really, the slow thing is the bytes, not the undef mask

Wesley Wiser (Dec 27 2019 at 16:22, on Zulip):

Like, what if instead of reading 1 byte at a time, we read 4 bytes or 8 or something.

Wesley Wiser (Dec 27 2019 at 16:22, on Zulip):

I assume there's some constant overhead to reading any number of bytes

Wesley Wiser (Dec 27 2019 at 16:22, on Zulip):

Could we reduce that by reading more at a time?

oli (Dec 27 2019 at 16:22, on Zulip):

Ah, true

oli (Dec 27 2019 at 16:23, on Zulip):

But that's an orthogonal optimization

Wesley Wiser (Dec 27 2019 at 16:23, on Zulip):

Yeah

oli (Dec 27 2019 at 16:26, on Zulip):

Let's do that in a separate PR

oli (Dec 27 2019 at 16:27, on Zulip):

The "everything is undef, copy nothing" optimization is basically what is needed to undo the regression here

Santiago Pastorino (Dec 27 2019 at 16:29, on Zulip):

yeah, that seems better :)

Santiago Pastorino (Dec 27 2019 at 16:30, on Zulip):

@oli did you find a PR or something?

oli (Dec 27 2019 at 16:31, on Zulip):

jep: literally this second: https://github.com/rust-lang/rust/pull/62655

Santiago Pastorino (Dec 27 2019 at 16:32, on Zulip):

ok

Santiago Pastorino (Dec 27 2019 at 16:32, on Zulip):

going for lunch, maybe later I can provide a PR like that one

Santiago Pastorino (Dec 27 2019 at 16:33, on Zulip):

@oli let's close this https://github.com/rust-lang/rust/pull/67658 I guess?

oli (Dec 27 2019 at 16:33, on Zulip):

nono :D

oli (Dec 27 2019 at 16:33, on Zulip):

again, another orthogonal optimization

Santiago Pastorino (Dec 27 2019 at 16:35, on Zulip):

@Wesley Wiser do you want to proceed with the reading more bytes idea? or should I?

Wesley Wiser (Dec 27 2019 at 16:36, on Zulip):

If you have other stuff to work on, I don't mind taking it

oli (Dec 27 2019 at 16:36, on Zulip):

@Santiago Pastorino if you want to test your zst fix PR, you can add https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=dd34e88a742b93dad4433bb46e22a666 to the ui test suite :D

oli (Dec 27 2019 at 16:36, on Zulip):

basically this is a canary that will just never finish compiling

oli (Dec 27 2019 at 16:37, on Zulip):

well, maybe not just the test, but add a comment that the test should compile in microseconds

Wesley Wiser (Dec 27 2019 at 16:37, on Zulip):

@oli Should we r- your PR to work around the const prop issue since the rollup failed?

oli (Dec 27 2019 at 16:37, on Zulip):

yes

Santiago Pastorino (Dec 27 2019 at 16:39, on Zulip):

If you have other stuff to work on, I don't mind taking it

please do

Wesley Wiser (Dec 27 2019 at 17:12, on Zulip):

/me went out to get some lunch & is back now.

oli (Dec 27 2019 at 17:21, on Zulip):

I couldn't find any MIR stuff to do so I'm fixing match ICEs around constants in patterns now

Wesley Wiser (Dec 27 2019 at 17:24, on Zulip):

One maybe kind of large project would be to do something about this hack in the Inliner https://github.com/rust-lang/rust/issues/67557#issuecomment-568746201 which is pretty annoying and means we miss a lot of inlining opportunity within a module.

oli (Dec 27 2019 at 17:26, on Zulip):

XD yea that hack is horrible

Wesley Wiser (Dec 27 2019 at 17:28, on Zulip):

I swear I've wasted at least 4 hours trying to figure out why a mir-opt test isn't working only to realize I need to move one function above another

oli (Dec 27 2019 at 17:28, on Zulip):

:frown:

oli (Dec 27 2019 at 17:31, on Zulip):

Without touching the query system to detect cycles, the only fix idea I've had so far is to add a has_function_call_to(body: DefId, check: DefId) -> bool query that tells you whether body's mir::Body contains a TerminatorKind call to check, recursively

oli (Dec 27 2019 at 17:31, on Zulip):

though then you'd need a &[DefId] for check, because you need to build up the call stack

oli (Dec 27 2019 at 17:32, on Zulip):

for better caching Set<DefId> would make more sense than &[DefId], though using that as a query key may be... bad

Wesley Wiser (Dec 27 2019 at 17:35, on Zulip):

This is not ideal, but what if we ran all the other optimizations except the inliner and made a query for that? We could then use that query in the inliner without cycles.

oli (Dec 27 2019 at 17:36, on Zulip):

the other optimizations may be more effective after inlining

Wesley Wiser (Dec 27 2019 at 17:36, on Zulip):

We'd of course miss things but we don't run the inliner currently anyway for normal builds so we may be in a better state than we are now

Wesley Wiser (Dec 27 2019 at 17:36, on Zulip):

Yeah

Santiago Pastorino (Dec 27 2019 at 19:01, on Zulip):

for the reported issue ...

Santiago Pastorino (Dec 27 2019 at 19:01, on Zulip):
[santiago@galago myapp (master)]$ hyperfine --warmup 3 'rustc src/main.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs'
Benchmark #1: rustc src/main.rs
  Time (mean ± σ):     16.157 s ±  1.099 s    [User: 16.014 s, System: 0.061 s]
  Range (min … max):   14.315 s … 17.247 s    10 runs

Benchmark #2: ~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs
  Time (mean ± σ):     13.251 s ±  0.651 s    [User: 13.132 s, System: 0.058 s]
  Range (min … max):   12.470 s … 14.153 s    10 runs

Summary
  '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs' ran
    1.22 ± 0.10 times faster than 'rustc src/main.rs'
Santiago Pastorino (Dec 27 2019 at 19:02, on Zulip):

the PR is 22% faster

Santiago Pastorino (Dec 27 2019 at 19:02, on Zulip):

that has exercised this example ...

Santiago Pastorino (Dec 27 2019 at 19:02, on Zulip):
fn main() {
    let _ = [(); 100_000_000];
}
Santiago Pastorino (Dec 27 2019 at 19:05, on Zulip):

and for the ctfe stress test we get ...

Santiago Pastorino (Dec 27 2019 at 19:05, on Zulip):
[santiago@galago myapp (master)]$ hyperfine --warmup 3 'rustc src/main.rs' '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs'
Benchmark #1: rustc src/main.rs
  Time (mean ± σ):      1.650 s ±  0.158 s    [User: 1.352 s, System: 0.337 s]
  Range (min … max):    1.334 s …  1.814 s    10 runs

Benchmark #2: ~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs
  Time (mean ± σ):      2.689 s ±  0.470 s    [User: 2.483 s, System: 0.237 s]
  Range (min … max):    2.272 s …  3.609 s    10 runs

Summary
  'rustc src/main.rs' ran
    1.63 ± 0.32 times faster than '~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs'
Santiago Pastorino (Dec 27 2019 at 19:06, on Zulip):

63% slower so no improvements at all

Santiago Pastorino (Dec 27 2019 at 19:06, on Zulip):

can check against current master quickly

oli (Dec 27 2019 at 19:44, on Zulip):

Hmmm

Wesley Wiser (Dec 27 2019 at 19:45, on Zulip):

@Santiago Pastorino and I paired on this on a call and we've got this cut-down repro:

fn main() {
  let _ = [(); 100_000_000];
}

from ~9 seconds on my machine to < 0.3.

Wesley Wiser (Dec 27 2019 at 19:45, on Zulip):

The fix is #67667

Wesley Wiser (Dec 27 2019 at 19:49, on Zulip):

I've got some stuff around the house I need to go do so I'm signing off. This was a lot of fun and I'm looking forward tomorrow! :slight_smile:

oli (Dec 27 2019 at 19:49, on Zulip):

Well, that fix doesn't work for struct Foo;... Maybe compute the layout of tys (the element type) before the match and report the uninhabited error if. ...
OK, see you tomorrow

oli (Dec 27 2019 at 19:50, on Zulip):

... the layout is uninhabited and otherwise skip validation if the type is zst

Wesley Wiser (Dec 27 2019 at 19:50, on Zulip):

Well, that fix doesn't work for struct Foo;

Are you sure?

wesley@endurance:~/code/rust/rust> cat test.rs
#[derive(Clone, Copy)]
struct Foo;

fn main() {
    let _ = [Foo; 4_000_000_000];
}
wesley@endurance:~/code/rust/rust> time rustc +stage1 --emit mir test.rs

real    0m0.084s
user    0m0.051s
sys     0m0.012s
oli (Dec 27 2019 at 19:51, on Zulip):

Huh??

oli (Dec 27 2019 at 19:51, on Zulip):

Oh

oli (Dec 27 2019 at 19:51, on Zulip):

Then struct Foo(());

Wesley Wiser (Dec 27 2019 at 19:51, on Zulip):

That's fair

Santiago Pastorino (Dec 27 2019 at 19:52, on Zulip):

still the ctfe-stress test I guess is not fixed by any of these fixes ...

Wesley Wiser (Dec 27 2019 at 19:52, on Zulip):

I'll change it later to use the layout or at worst case tomorrow morning

oli (Dec 27 2019 at 19:52, on Zulip):

No rush

oli (Dec 27 2019 at 19:52, on Zulip):

New party tomorrow

Wesley Wiser (Dec 27 2019 at 19:52, on Zulip):

:wave:

oli (Dec 27 2019 at 20:00, on Zulip):

Quite weird. That constant being copied shouldn't actually cause any copying for allocations full of undefs

oli (Dec 27 2019 at 20:02, on Zulip):

Maybe the memory ends up being marked as initialized out of some reason

oli (Dec 27 2019 at 20:02, on Zulip):

Though I still can't fathom how that happens

Santiago Pastorino (Dec 28 2019 at 09:55, on Zulip):

morning :wave:

Last update: Jan 28 2020 at 00:40UTC