Stream: t-compiler

Topic: Suppress drop terminators for fields in other variants


ecstatic-morse (Jan 21 2020 at 03:56, on Zulip):

@eddyb I looked into your idea for making const fn unwrap work a bit tonight. It looks like we currently propagate dataflow state along the imaginary edge of a FalseEdges terminator. We would need to stop doing this, otherwise it will pollute the entry set of the None branch. For example, this code:

fn unwrap<T>(opt: Option<T>) -> T {
    match opt {
        Some(inner) => inner,
        None => std::process::abort(),
    }
}

results in the following CFG

false-edges.png output.svg

ecstatic-morse (Jan 21 2020 at 04:00, on Zulip):

However, I assume the whole point of the FalseEdges terminator is to propagate dataflow state into other basic blocks, so maybe this is a no-go. BTW, bb3 is the None branch.

ecstatic-morse (Jan 21 2020 at 04:01, on Zulip):

@eddyb Is there a way to work around this that I'm not seeing?

ecstatic-morse (Jan 21 2020 at 04:08, on Zulip):

Reading a bit, it seems like FalseEdges are needed becasue of pattern guards. Perhaps we can stop emitting them when no guard is present?

ecstatic-morse (Jan 21 2020 at 04:08, on Zulip):

Sorry about the blurriness, it looks like SVG isn't supported.

ecstatic-morse (Jan 21 2020 at 04:09, on Zulip):

output.pdf

ecstatic-morse (Jan 21 2020 at 04:19, on Zulip):

I just noticed that, in bb5,drop is being called on _3 right after it is moved into the return place. This probably means that I'm looking at MIR that's too early in the pipeline (this is the MIR that const qualification currently runs on).

ecstatic-morse (Jan 21 2020 at 04:26, on Zulip):

Okay, yeah I needed to be looking at the post-ElaborateDrops MIR.

ecstatic-morse (Jan 21 2020 at 04:27, on Zulip):

output.pdf

ElaborateDrops.after

ecstatic-morse (Jan 21 2020 at 05:07, on Zulip):

So now that I'm actually looking at the right stage. It seems like there's a slightly different problem. We check the discriminant of opt (_1), then if it's Some we jump to bb4, which puts the value inside the option inside the return place. However, at the end of bb4, we check the discriminant again and drop(_1) if it has magically changed out from under us.

It seems we need to do some more dead code elimination after elaborate drops.

ecstatic-morse (Jan 21 2020 at 05:14, on Zulip):

One other thing I noticed is that we don't actually do ElaborateDropson generic functions unless they are monomorphized. I think this means that we can't rely on ElaborateDrops to forbid reachable Drop terminators since we want pre-monomorphization errors.

ecstatic-morse (Jan 21 2020 at 05:14, on Zulip):

(btw, I'm sure a lot of the things I'm saying are obvious to you, I'm thinking out loud atm)

ecstatic-morse (Jan 21 2020 at 05:34, on Zulip):

So resetting a bit: It looks like we'll need to deal with this MIR. We don't need to worry about the drop of _3 in bb5 because check_consts already looks for when a local is moved out of prior to being dropped. It's the drop of _1 in bb7 that is causing problems. I need to replicate whatever part of elaborate_drops is responsible for realizing that _1 no longer needs to be dropped after we move out of it in bb5[1].

ecstatic-morse (Jan 21 2020 at 05:36, on Zulip):

oli mentioned in #66753 that the None case was also causing problems. That's actually not true; I should have double-checked their comment at the time. The drop they refer to is in a cleanup block, so it is ignored by const-checking. The rationale being that unwinding during const-eval is basically an abort that triggers a compilation failure.

oli (Jan 21 2020 at 06:53, on Zulip):

It's not in cleanup. It's reachable via bb0 -> bb2 -> bb3 -> bb17 -> bb7 -> bb8 -> bb16

oli (Jan 21 2020 at 06:54, on Zulip):

basically after the bar() call there's a match x { Some(_) => {}, None => drop(x) } (the switch in bb8 does this)

ecstatic-morse (Jan 21 2020 at 08:42, on Zulip):

Ach you're right (for some reason I saw bb11 on your GH comment). It does look like there's no non-cleanup drop terminator on the None side for unwrap.

fn unwrap<T>(opt: Option<T>) -> T {
    match opt {
        Some(inner) => inner,
        None => std::process::abort(),
    }
}
ecstatic-morse (Jan 21 2020 at 08:49, on Zulip):

So if the goal is to get exactly unwrap and expect working in const fns, I think we could do it by using the results of maybe_initsto detect when all fields of an ADT are uninitialized at a certain location.

ecstatic-morse (Jan 21 2020 at 08:53, on Zulip):

I'm not sure which of the categories you mentioned in your comment this belongs to. Maybe "independent solution"? I also think we should talk about what our long-term goals are. How brittle a system are we comfortable given that someday people will be able to write T: const Drop (not actual syntax) to opt out of this in many cases?

ecstatic-morse (Jan 21 2020 at 08:56, on Zulip):

I will also look into the example you posted oli, to see if I can apply eddyb's ideas about per-edge effects from SwitchInts to remove some unnecessary drops from the final MIR.

ecstatic-morse (Jan 21 2020 at 09:25, on Zulip):

I should go to bed. I'm rambling.

eddyb (Jan 21 2020 at 17:51, on Zulip):

at the very least we might be able to get simpler codegen (since I think today we rely on LLVM to remove the drop)

ecstatic-morse (Jan 22 2020 at 20:24, on Zulip):

Okay, I did not do a good job of identifying and explaining the various concerns here. I'll have another go:

ecstatic-morse (Jan 22 2020 at 20:26, on Zulip):

For unwrap, the only Drop terminator that's not in a cleanup block is along the Some edge. AFAICT, there's two shortcomings with drop elaboration currently. The first, which @eddyb pointed out, is that MaybeInitializedPlacesand drop elaboration doesn't incorporate the information we get when we switch on a discriminant. As a result, oli's example, which is slightly more complex than unwrap, does have a non-cleanup Drop along the None path.

ecstatic-morse (Jan 22 2020 at 20:30, on Zulip):

The second shortcoming is that when we move out of all fields of an ADT without custom Drop glue, we still emit a Drop terminator for that ADT. This occurs along the Some branch of unwrap. I posted some erroneous MIR diagrams for unwrap that didn't show this, which were generated from an experimental branch into which I had introduced a bug. Here's an accurate diagram of the MIR for unwrap after drop elaboration (and cleanup)

pasted image

ecstatic-morse (Jan 22 2020 at 20:34, on Zulip):

And here's the MIR before drop elaboration.

pasted image

ecstatic-morse (Jan 22 2020 at 20:36, on Zulip):

We end up checking the discriminant of _1 twice because the drop(_1) that is originally in basic block 6 is classified as an "open" drop by drop elaboration because some fields of _1 have been moved out of.

ecstatic-morse (Jan 22 2020 at 20:39, on Zulip):

I want to stop classifying drops of locals without custom drop glue that have all of their fields moved out as "open". I haven't fully considered the ramifications of this, but it seems like it will be tractable. Another approach would be const-propagation for calls to discriminant along with DCE, but this would have to occur very late in the pipeline and would not be accessible during const-checking.

ecstatic-morse (Jan 22 2020 at 21:12, on Zulip):

To detect when all fields have been moved out of the argument to a Drop terminator, I would need to:

ecstatic-morse (Jan 22 2020 at 21:17, on Zulip):

For enums, we would also need to fix the first shortcoming (marking fields that are not in the live variant as uninit) for this to work generally. Option is a bit of a special case, as it has exactly one field across all of its variants, so it's not dependent on this.

ecstatic-morse (Jan 22 2020 at 21:20, on Zulip):

There's a bit of extra complexity because three move paths are involved when moving out of Some: _1, _1 as Some, and (_1 as Some).0. Only the last one is marked as uninitialized.

ecstatic-morse (Jan 23 2020 at 20:48, on Zulip):

#68493 should cause drop elaboration to stop emitting spurious Drop terminators when a value is moved from Option::Some(_). I'll start working on eddyb's idea next, which would result in a similar improvement for enums that have more than one field across all variants (e.g., Result).

ecstatic-morse (Jan 23 2020 at 22:13, on Zulip):

Hmm, my one-line change results in incorrect codegen for unwrap_or. It doesn't clear the drop flag for (_1 as Some).0 when taking the None branch. I'll need to do some more research about how this is supposed to work.

centril (Jan 23 2020 at 22:38, on Zulip):

@ecstatic-morse does that PR affect static analysis as well?

Matthew Jasper (Jan 23 2020 at 22:39, on Zulip):

It doesn't

centril (Jan 23 2020 at 22:41, on Zulip):

ah, good; then I think I followed things right

ecstatic-morse (Jan 23 2020 at 22:43, on Zulip):

@Matthew Jasper do you happen to know where I could find info on the high-level design of drop elaboration? Normally I use git blame and look at the PR summary, but the commit for most of elaborate_drops was the one that moved it into librustc_mir.

ecstatic-morse (Jan 23 2020 at 22:44, on Zulip):

I will be able to fumble through just from reading the code, but I'm pretty slow.

Matthew Jasper (Jan 23 2020 at 22:45, on Zulip):

Elaborate drops is old and undocumented. Comment in the source are probably all that exists. Maybe (but probably not) there's something in the Rustc guide.

ecstatic-morse (Jan 23 2020 at 22:45, on Zulip):

Yeah, I checked the guide as well and didn't find anything :confused:

centril (Jan 23 2020 at 22:46, on Zulip):

I was wondering whether https://github.com/rust-lang/reference/pull/514 would be useful, but that's probably unrelated?

Matthew Jasper (Jan 23 2020 at 22:46, on Zulip):

It's unrelated, unfortunately.

centril (Jan 23 2020 at 22:47, on Zulip):

(Also that lovely PR really needs to get merged, but that's also unrelated, but cc @pnkfelix)

ecstatic-morse (Jan 23 2020 at 22:53, on Zulip):

#33622 (found via transitive git blame) is the genesis of drop elaboration. I wonder if @Ariel Ben-Yehuda has any thoughts on how to be more efficient around enums? This was implemented in 2016 and I don't remember what I had for lunch on Monday, so I would guess no XD.

eddyb (Jan 27 2020 at 12:46, on Zulip):

@ecstatic-morse I don't understand how there can only be a Drop on the Some branch

eddyb (Jan 27 2020 at 12:46, on Zulip):

oh, unwrap

eddyb (Jan 27 2020 at 12:48, on Zulip):

@ecstatic-morse I understand my confusion now, at some point I stopped thinking of Option::unwrap and more of a match in general (you're right about unwrap_or, it's a better example of what I was imagining)

ecstatic-morse (Jan 29 2020 at 08:19, on Zulip):

Sorry @eddyb, I had some distractions the past few days. I think I have the proper modifications in drop elaboration to better handle fieldless enum variants in #68528 now. You can see the effects on the MIR for Option::unwrap_or below.

Before

After

Even without drop elaboration, that PR is a pretty significant performance win; if the top-line numbers for syn-opt are anywhere close to reality I would be overjoyed.

eddyb (Jan 29 2020 at 15:52, on Zulip):

-9% on syn-opt?!

eddyb (Jan 29 2020 at 15:52, on Zulip):

this feels too good to be true

ecstatic-morse (Jan 29 2020 at 17:44, on Zulip):

Agreed. I'm not sure how to verify that I've not introduced a miscompilation. that PR also includes my changes to data flow which is a small perf gain on its own. I expected to see a small regression in check builds and a small boost in codegened builds, but 10% is beyond the pale.

ecstatic-morse (Jan 29 2020 at 18:48, on Zulip):

I added a proper summary to #68528

Zoxc (Jan 29 2020 at 20:05, on Zulip):

There are two large regressions though, https://perf.rust-lang.org/compare.html?start=d55f3e9f1da631c636b54a7c22c1caccbe4bf0db&end=0077a7aa11ebc2462851676f9f464d5221b17d6a&stat=wall-time

ecstatic-morse (Jan 29 2020 at 20:13, on Zulip):

@Zoxc I suspect there is a lot of variance for wall-time on clean incremental builds since they finish so quickly, although the "noise run" link is broken at the top of that page.

ecstatic-morse (Jan 29 2020 at 20:38, on Zulip):

cc @simulacrum ^

simulacrum (Jan 29 2020 at 20:39, on Zulip):

Compare to https://perf.rust-lang.org/compare.html?start=a237641c7df8125b89b8f9c2a3594964ba8188f8&end=c3681d62ee4bb849e87a2ec5d663afc34ac05d85&stat=wall-time, but yes, I would expect high variance

simulacrum (Jan 29 2020 at 20:40, on Zulip):

(that's a "noop" build)

lqd (Jan 29 2020 at 20:41, on Zulip):

this new perf run with a +0.1% and -0.7% instructions resulting in +800ms seems high variance, and those numbers were actually reversed on the previous perf run

lqd (Jan 29 2020 at 20:42, on Zulip):

the "noop build" has a 47% regression in wall-time on cranelift ? :)

ecstatic-morse (Jan 29 2020 at 20:44, on Zulip):

It appears that syn-opt also has some variance in I-count measurements as well. I suspect that my results for syn-opt are the combination of a legitimate perf gain with some helpful variance (hence the question mark).

ecstatic-morse (Jan 29 2020 at 20:48, on Zulip):

I'm quite happy with 2-4% improvements in wall time for a lot of real-world crates. I wonder what's up with tokio-webpush-simple-opt though. Maybe I've done something that's unsound in the presence of generators?

Zoxc (Jan 29 2020 at 20:50, on Zulip):

@simulacrum That's a lot of variance on that benchmark

simulacrum (Jan 29 2020 at 20:51, on Zulip):

Yes? I mean, what do you expect?

simulacrum (Jan 29 2020 at 20:51, on Zulip):

this is measuring wall time from probably one or two runs

simulacrum (Jan 29 2020 at 20:52, on Zulip):

for something that takes ~2 seconds, and is highly dependent on disk performance I imagine

nagisa (Jan 30 2020 at 02:21, on Zulip):

That new mir graph looks glorious.

Last update: Jun 07 2020 at 09:45UTC