Stream: t-compiler/wg-mir-opt

Topic: Specialization causes mis-optimizations


Wesley Wiser (Dec 23 2019 at 17:33, on Zulip):

So it looks like @eddyb was absolutely correct here when they suggested that Instance::resolve() is not correctly handling default trait items. I took their suggestion and added the logic to Instance::resolve() in this branch. That fixes the bad optimizations (issues in Inline, ConstProp, and also SimplifyBranches) but it breaks a few UI tests and I'm not exactly sure why.

failures:
    [ui] ui/associated-consts/associated-const-cross-crate-defaults.rs
    [ui] ui/associated-consts/associated-const-use-default.rs
    [ui] ui/consts/miri_unleashed/assoc_const.rs
    [ui] ui/consts/miri_unleashed/assoc_const_2.rs
    [ui] ui/consts/trait_specialization.rs
    [ui] ui/custom_test_frameworks/dynamic.rs
    [ui] ui/issues/issue-11205.rs
    [ui] ui/rfcs/rfc-2005-default-binding-mode/constref.rs
    [ui] ui/ufcs-polymorphic-paths.rs

The most common failure is this assertion failing. cc @oli I think you authored this.

I believe the issue is that with my patch Reveal::UserFacing can cause Instance::resolve() to return None where with Reveal::All it would return a default impl.

Wesley Wiser (Dec 23 2019 at 17:33, on Zulip):

I guess my question is: why does the calling code use Reveal::All but here we change it to use Reveal::UserFacing?

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

Oh wait, the comment in const_eval_raw_provider() does explain this...

Wesley Wiser (Dec 23 2019 at 17:37, on Zulip):

I guess this code needs to be updated to handle TooGeneric differently because TooGeneric when Reveal::UserFacing does not necessarily mean it will fail when Reveal::All is used.

oli (Dec 23 2019 at 17:39, on Zulip):

oh, yea that assert makes no sense this way

oli (Dec 23 2019 at 17:40, on Zulip):

feel free to remove it without a replacement

Wesley Wiser (Dec 23 2019 at 17:40, on Zulip):

Won't that re-introduce #55454?

oli (Dec 23 2019 at 17:41, on Zulip):

No, this assert is not load bearing. It's just something I assumed to never happen

Wesley Wiser (Dec 23 2019 at 17:42, on Zulip):

Ok, gotcha. Looks like there's a covering test so I can remove and make sure those tests still pass to verify nothing else breaks.

oli (Dec 23 2019 at 17:44, on Zulip):

well, the test isn't ICEing, so the assert can't have an effect on it

Wesley Wiser (Dec 23 2019 at 17:44, on Zulip):

Ah right

oli (Dec 23 2019 at 17:45, on Zulip):

but if you want to keep the assert, you can put it in https://github.com/rust-lang/rust/blob/a916ac22b9f7f1f0f7aba0a41a789b3ecd765018/src/librustc_mir/const_eval.rs#L784 (assert that key.param_env.reveal != Reveal::All || err_inval!(TooGeneric) != err.error)

Wesley Wiser (Dec 23 2019 at 17:47, on Zulip):

9 test failures -> 5

Wesley Wiser (Dec 23 2019 at 17:48, on Zulip):

There's an LLVM failure I have no idea how to debug so I'm going to leave that for last and hope it get's fixed along the way :)

oli (Dec 23 2019 at 17:52, on Zulip):

Not sure what ui test failures you're getting, but maybe some are legit fixes?

Wesley Wiser (Dec 23 2019 at 17:55, on Zulip):

Down to 2, I'll post the details when this build finishes

Wesley Wiser (Dec 23 2019 at 17:59, on Zulip):
failures:

---- [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`.

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


---- [ui] ui/ufcs-polymorphic-paths.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 101
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/ufcs-polymorphic-paths.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/ufcs-polymorphic-paths/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/ufcs-polymorphic-paths/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
Global is external, but doesn't have external or weak linkage!
i64 ()** @_ZN22ufcs_polymorphic_paths4main1S17h617838d98e0a145aE
Global is external, but doesn't have external or weak linkage!
i64 ()** @_ZN22ufcs_polymorphic_paths4main1S17h6d6a871d84675c61E
Global is external, but doesn't have external or weak linkage!
i64 ()** @_ZN22ufcs_polymorphic_paths4main1S17h08975e9ef2b0831eE
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17h348875713cbbb989E
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17h4e60c7352cd11b4aE
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17h5ff48db54f3efa7aE
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17hf6a8ed03bf7edb7eE
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17hdf6a847ad5e63ca9E
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17h93c1cfcdf807a32bE
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17h49ed8eee2ac98a8fE
Global is external, but doesn't have external or weak linkage!
i32 (%XorShiftRng*)** @_ZN22ufcs_polymorphic_paths4main1S17h000c406925f8c1b6E
LLVM ERROR: Broken module found, compilation aborted!

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



failures:
    [ui] ui/rfcs/rfc-2005-default-binding-mode/constref.rs
    [ui] ui/ufcs-polymorphic-paths.rs

test result: FAILED. 9381 passed; 2 failed; 45 ignored; 0 measured; 0 filtered out
Wesley Wiser (Dec 23 2019 at 19:53, on Zulip):

I think I resolved the constref.rs test but I'm not 100% sure about the changes. The ufcs-polymorphic-paths test continues to elude me...

Wesley Wiser (Dec 24 2019 at 13:53, on Zulip):

I'm starting to wonder if there's something wrong with my llvm build:

wesley@endurance:~/code/rust/rust> rustc +stage1 src/test/ui/ufcs-polymorphic-paths.rs
Global is external, but doesn't have external or weak linkage!
i64 ()** @_ZN22ufcs_polymorphic_paths4main1S17h0036ecaaa6b24075E
LLVM ERROR: Broken module found, compilation aborted!
LLVM ERROR: " Ȇ�DVnspecified" pass is not registered.
oli (Dec 24 2019 at 15:50, on Zulip):

maybe we're missing a few things because we don't collect them correctly now?

oli (Dec 24 2019 at 15:50, on Zulip):

like if we forget to adjust substs correctly I'd imagine the collector to miss some items, but they'll still be referred to

oli (Dec 24 2019 at 15:51, on Zulip):

we've had problems like this with exposed bodies before

Wesley Wiser (Dec 24 2019 at 15:55, on Zulip):

Maybe?

Wesley Wiser (Dec 24 2019 at 16:03, on Zulip):

I dumped the LLVM IR and here's the diff (minus unimportant stuff like hash changes):

Before:

@_ZN22ufcs_polymorphic_paths4main1S17h0036ecaaa6b24075E = internal constant <{ i8*, [0 x i8] }> <{ i8* bitcast (i64 ()* @_ZN22ufcs_polymorphic_paths4Size4size17hb2d75803bb586561E to i8*), [0 x i8] zeroinitializer }>, align 8

; ufcs_polymorphic_paths::Size::size
; Function Attrs: nonlazybind uwtable
define internal i64 @_ZN22ufcs_polymorphic_paths4Size4size17hb2d75803bb586561E() unnamed_addr #0 {
start:
  %0 = alloca i64, align 8
  store i64 1, i64* %0, align 8
  %1 = load i64, i64* %0, align 8
  br label %bb1

bb1:                                              ; preds = %start
  ret i64 %1
}

After:

@_ZN22ufcs_polymorphic_paths4main1S17h0036ecaaa6b24075E = internal global i64 ()*
Wesley Wiser (Dec 24 2019 at 16:03, on Zulip):

So that seems like a strong possibility

oli (Dec 26 2019 at 02:38, on Zulip):

I tried to look into this by deduplicating the bail out logic in https://github.com/oli-obk/rust/pull/new/instance_resolve_specialization to see if there are any differences between assemble_candidates_from_impls and resolve

oli (Dec 26 2019 at 02:38, on Zulip):

But it seems like my scheme won't work

oli (Dec 26 2019 at 02:39, on Zulip):

but I noticed two things: the changes to ui on the "erroneous constant" tests are odd

oli (Dec 26 2019 at 02:39, on Zulip):

they used to get reported in const prop

oli (Dec 26 2019 at 02:39, on Zulip):

now they get reported at monomorphization time in codegen

oli (Dec 26 2019 at 02:39, on Zulip):

but the function was already monomorphic

oli (Dec 26 2019 at 02:39, on Zulip):

so shouldn't it keep getting reported from const prop?

Wesley Wiser (Dec 26 2019 at 02:42, on Zulip):

Hmm

oli (Dec 26 2019 at 02:42, on Zulip):

the other thing I noticed: I can cause metadata loading to ICE if I make confirm_impl_candidate also succeed with the same strategy used in assemble_candidates_from_impls

Wesley Wiser (Dec 26 2019 at 02:48, on Zulip):

ut I noticed two things: the changes to ui on the "erroneous constant" tests are odd

Something interesting I'm looking into with the ufcs test is that it appears to be related to a constant being missed during codegen

anp (Dec 26 2019 at 13:23, on Zulip):

:eyes: since i think i'm blocked on https://github.com/rust-lang/rust/pull/67137#issuecomment-568655485 bc of this

oli (Dec 26 2019 at 13:28, on Zulip):

@anp I found something: I think in your situation the bug is actually that https://github.com/rust-lang/rust/blob/acb6690e1d58fc5f262ada5b5030fe73e601f1e8/src/librustc_mir/transform/const_prop.rs#L414 ignores https://github.com/rust-lang/rust/blob/a916ac22b9f7f1f0f7aba0a41a789b3ecd765018/src/librustc_mir/interpret/operand.rs#L565-L566

oli (Dec 26 2019 at 13:28, on Zulip):

The constant has not yet been monomorphized

anp (Dec 26 2019 at 13:28, on Zulip):

interesting

oli (Dec 26 2019 at 13:29, on Zulip):

@Wesley Wiser this may in fact be what is causing the "erroneous constant" tests to change

oli (Dec 26 2019 at 13:29, on Zulip):

@anp I'll open a rustc PR, then you can rebase over it

anp (Dec 26 2019 at 13:31, on Zulip):

:heart:

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

ok, that should do it. lemme know if these tests still give you trouble

anp (Dec 26 2019 at 14:29, on Zulip):

no dice :frown:

anp (Dec 26 2019 at 14:30, on Zulip):

anything you want me to try?

oli (Dec 26 2019 at 15:52, on Zulip):

hmm... can you try dumping the MIR at ConstProp.before for the test?

oli (Dec 26 2019 at 15:56, on Zulip):

also a backtrace by compiling the test with -Ztreat-err-as-bug could help pinpoint the source

oli (Dec 26 2019 at 15:58, on Zulip):

@anp actually, I may have done a stupid and not actually affected the code that evaluates the constant in your case. Try adding the same bailout check before https://github.com/rust-lang/rust/blob/acb6690e1d58fc5f262ada5b5030fe73e601f1e8/src/librustc_mir/transform/const_prop.rs#L561 but for rvalue

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

I pushed an update

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

you can try another rebase

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

I wish hub had hub pr rebase and not just hub pr checkout

anp (Dec 26 2019 at 17:51, on Zulip):

thanks! i’m in the office now and don’t have my rustc work checked out on this machine, will try in a few hours when the temp on our floor drives me back home

anp (Dec 26 2019 at 19:14, on Zulip):

that fixed it! woohoo :D

oli (Dec 26 2019 at 19:24, on Zulip):

awesome!

anp (Dec 27 2019 at 02:08, on Zulip):

i think https://github.com/rust-lang/rust/pull/67631 may need to be r+'d again after the failed rollup?

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

Rollups are manual so it should be ok :)

anp (Dec 27 2019 at 04:39, on Zulip):

oh i must have missed it when i checked homu earlier, woops!

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

I just talked with @eddyb maybe we should continue trying to pursue the branch that I started in https://github.com/oli-obk/rust/pull/new/instance_resolve_specialization

anp (Dec 27 2019 at 18:16, on Zulip):

@Wesley Wiser i'm around on&off today if you want me to poke at stuff related to 67662

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

I have an idea how we can have our :cake: and eat it, too

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

Wonderful!

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

I'll make my PR turn the hard error into a lint, then you can warn(const_err) in the test

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

it's hacky, but it allows us to make the hard error change later while giving ppl a heads up that this is problematic now

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

Doesn't it already fall into the const_prop error handling machinery?

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

Or is because it's a cross crate macro that we have to do something special?

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

no it's because using the constant will cause a hard error

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

https://github.com/rust-lang/rust/blob/41501a6b03a8f10d8c29dfcb37dbd5ff84b33f34/src/librustc_mir/transform/const_prop.rs#L418

Santiago Pastorino (Dec 27 2019 at 18:26, 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 ± σ):     12.090 s ±  1.243 s    [User: 11.974 s, System: 0.045 s]
  Range (min … max):    9.891 s … 14.533 s    10 runs

Benchmark #2: ~/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/main.rs
  Time (mean ± σ):     14.321 s ±  1.798 s    [User: 14.200 s, System: 0.053 s]
  Range (min … max):   12.783 s … 17.701 s    10 runs

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

@oli this is on my example

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

both patches

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

I think that's way better

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

definitely

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

still 18% slower but at least not 50%

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

:joy:

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

hehe

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

I think we can work with this

anp (Dec 27 2019 at 22:30, on Zulip):

@Wesley Wiser can you r+ 67631 again?

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

no

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

wait :D

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

I'm fixing it up for the warning stuff

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

sorry didn't get to that yet, but I have time now and should have something within the hour

anp (Dec 27 2019 at 23:01, on Zulip):

oh cool!

oli (Dec 28 2019 at 00:49, on Zulip):

Tests are running, going to bed now

anp (Dec 28 2019 at 17:12, on Zulip):

how'd they fare

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

badly, fighting with the interner

anp (Dec 28 2019 at 17:33, on Zulip):

and people talk about fighting borrowck

Last update: Apr 05 2020 at 01:25UTC