Stream: t-compiler/help

Topic: Making `unconditional_recursion` work across function calls


Jonas Schievink (Aug 01 2020 at 14:34, on Zulip):

It's that time of the year again, so I have started to look into #57965 again (seriously this time). I think I have most of the linting logic figured out, but am now hitting issues where it tried to read from stolen MIR.

I have split up mir_built into two queries to be able to run the lint right after MIR building (just like it is now), but without introducing cycles where the lint requests MIR of a called function, which then runs the lint on that function, which then requests the MIR of the original caller again. I'm not sure if this is the best way to do it, but it should work.

The problem now is that MIR may be optimized or otherwise analyzed, which steals the MIR from the mir_built query. If some unrelated function is then linted, the unoptimized MIR will be requested, which has been stolen. Any suggestions for getting around this?

Jonas Schievink (Aug 01 2020 at 14:56, on Zulip):

I guess I can add Steal::is_stolen and fall back to optimized_mir, which should always exist then...

oli (Aug 01 2020 at 15:04, on Zulip):

@Jonas Schievink you're solving the same problem as I'm solving for inlining

Jonas Schievink (Aug 01 2020 at 15:04, on Zulip):

I think so, yes

oli (Aug 01 2020 at 15:05, on Zulip):

There's an impl in https://github.com/rust-lang/rust/pull/68828

oli (Aug 01 2020 at 15:05, on Zulip):

which gives you all the info you are looking for

oli (Aug 01 2020 at 15:06, on Zulip):

then you can "just not" request the optimized_mir of a function if you know that's gonna cause a cycle

oli (Aug 01 2020 at 15:06, on Zulip):

and if mir_inliner_cycle returns None, you don't need to do anything anyway

Jonas Schievink (Aug 01 2020 at 15:07, on Zulip):

Ah, so does that just run inlining before any other optimizations?

oli (Aug 01 2020 at 15:07, on Zulip):

nope

oli (Aug 01 2020 at 15:07, on Zulip):

it computes the call graph before any other optimizations

oli (Aug 01 2020 at 15:07, on Zulip):

so as long as const prop or something else doesn't start devirtualizing function calls, we're fine

oli (Aug 01 2020 at 15:08, on Zulip):

and when we start devirtualizing we need some fancy scheming anyway

Jonas Schievink (Aug 01 2020 at 15:08, on Zulip):

oh, okay

oli (Aug 01 2020 at 15:09, on Zulip):

the perf results aren't too bad (up to 4% in some cases), so if we have two good uses cases for it, maybe we can argue for taking the perf hit

Jonas Schievink (Aug 01 2020 at 15:09, on Zulip):

i thought we could just solve inlining cycles by splitting the optimization pipeline into distinct queries

Jonas Schievink (Aug 01 2020 at 15:09, on Zulip):

and then use the early query before and in inlining, and the later query afterwards

oli (Aug 01 2020 at 15:09, on Zulip):

that's not too great, we'll want to run inlining and const prop in a loop until we reach a fixed point I think

oli (Aug 01 2020 at 15:10, on Zulip):

it also has the same problems wrt devirtualization

oli (Aug 01 2020 at 15:10, on Zulip):

and I have no idea on the perf implications

oli (Aug 01 2020 at 15:10, on Zulip):

maybe we should just request a compiler design meeting on this?

Jonas Schievink (Aug 01 2020 at 15:10, on Zulip):

hmm, yeah

Jonas Schievink (Aug 01 2020 at 15:10, on Zulip):

idk, I haven't looked into this much

oli (Aug 01 2020 at 15:11, on Zulip):

oh and I think the problem you're encountering now would still happen in the inlining split case ^^

Jonas Schievink (Aug 01 2020 at 15:11, on Zulip):

fixed-point inlining? that does sound cursed

oli (Aug 01 2020 at 15:12, on Zulip):

well... if we really want to take away work from llvm, we gotta do something like that

oli (Aug 01 2020 at 15:12, on Zulip):

we'll get a big gain just by normal inlining, but only if that inlining doesn't use the split scheme, otherwise we only ever inline a single level I think

Jonas Schievink (Aug 01 2020 at 18:52, on Zulip):
error: internal compiler error: src/librustc_mir/interpret/eval_context.rs:268:21: expected type differs from actual type.
expected: &mut std::string::String
actual: &std::string::String
  --> /home/jonas/dev/rust/src/test/ui/span/mut-arg-hint.rs:14:5
   |
LL | /     pub fn foo(mut a: &String) {
LL | |         a.push_str("foo"); //~ ERROR cannot borrow `*a` as mutable, as it is behind a `&` reference
LL | |     }
   | |_____^

no idea how this one's even possible

Jonas Schievink (Aug 01 2020 at 18:54, on Zulip):

oh, I guess requesting the optimized MIR is not a good idea when there's type errors

Jonas Schievink (Aug 01 2020 at 19:07, on Zulip):

Is there a way to find out if any fatal errors were reported? Session::has_errors will return true even if it just has errors because of a denyd lint.

oli (Aug 01 2020 at 19:13, on Zulip):

not sure if https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TypeckResults.html#structfield.tainted_by_errors suffices for your case

Jonas Schievink (Aug 01 2020 at 22:41, on Zulip):

that helps, yeah :)

Jonas Schievink (Aug 01 2020 at 22:43, on Zulip):

now the remaining issue to make this useful: Instance::resolve will ICE when it can't fully normalize the result

Jonas Schievink (Aug 01 2020 at 22:44, on Zulip):

this is a problem because I may pass semi-monomorphized substs to it that allow the instance to be resolved to a concrete function item, but that function might still contain generic types (and thus unnormalizable projections)

Jonas Schievink (Aug 01 2020 at 23:44, on Zulip):

oli said:

not sure if https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TypeckResults.html#structfield.tainted_by_errors suffices for your case

actually no, this still seems to give the same error

Jonas Schievink (Aug 02 2020 at 00:39, on Zulip):

okay so now I have

error: function cannot return without recursing
  --> $DIR/lint-unconditional-recursion.rs:152:1
   |
LL | fn cycle1() {
   | ^^^^^^^^^^^ cannot return without recursing
LL |     cycle2();
   |     -------- call into the next function in the cycle
...
LL |     cycle1();
   |     -------- call site completing the cycle
   |
note: next function in the cycle
  --> $DIR/lint-unconditional-recursion.rs:156:1
   |
LL | fn cycle2() {
   | ^^^^^^^^^^^

which looks odd, since the labels are getting reordered in a way that's hard to read. I'd like to have something like this:

error: function cannot return without recursing
  --> $DIR/lint-unconditional-recursion.rs:152:1
   |
LL | fn cycle1() {
   | ^^^^^^^^^^^ cannot return without recursing
LL |     cycle2();
   |     -------- call into the next function in the cycle
   |
note: next function in the cycle
  --> $DIR/lint-unconditional-recursion.rs:156:1
   |
LL | fn cycle2() {
   | ^^^^^^^^^^^
...
LL |     cycle1();
   |     -------- call site completing the cycle

I can't find a way to access the "sub"-diagnostic to attach labels. Is that possible?

Joshua Nelson (Aug 02 2020 at 00:47, on Zulip):

does https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.Diagnostic.html#method.highlighted_note with Style::LabelSecondary do what you want?

Joshua Nelson (Aug 02 2020 at 00:48, on Zulip):

@Jonas Schievink ^

Jonas Schievink (Aug 02 2020 at 00:48, on Zulip):

that seems to be for piecing a message together from parts

Jonas Schievink (Aug 02 2020 at 00:49, on Zulip):

and doesn't take a span

Joshua Nelson (Aug 02 2020 at 00:49, on Zulip):

ah hmm good point

Joshua Nelson (Aug 02 2020 at 00:50, on Zulip):

if you used span_note for 'call site completing the cycle' you could have the ordering be the way you want

Joshua Nelson (Aug 02 2020 at 00:51, on Zulip):
error: function cannot return without recursing
  --> $DIR/lint-unconditional-recursion.rs:152:1
   |
LL | fn cycle1() {
   | ^^^^^^^^^^^ cannot return without recursing
LL |     cycle2();
   |     -------- call into the next function in the cycle
   |
note: next function in the cycle
  --> $DIR/lint-unconditional-recursion.rs:156:1
   |
LL | fn cycle2() {
   | ^^^^^^^^^^^
...
note: call site completing the cycle
LL |     cycle1();
   |     --------
Jonas Schievink (Aug 02 2020 at 00:51, on Zulip):

yeah

Jonas Schievink (Aug 02 2020 at 00:51, on Zulip):

ah, wait, the span_note can be used with a MultiSpan, which includes labels

Jonas Schievink (Aug 02 2020 at 00:53, on Zulip):

seems fairly hard to integrate into what I have though

Jonas Schievink (Aug 02 2020 at 19:49, on Zulip):

opened #75067 for now, let's see if that even works without being horribly slow

cc @eddyb in case you're interested in this

oli (Aug 03 2020 at 06:38, on Zulip):

perfbot doesn't like it. I really think we should find a single scheme for inlining and unconditional_recursion

oli (Aug 03 2020 at 06:39, on Zulip):

While the current state of my PR has a 4% regression, we likely can figure something out to get rid of that, at which point the unconditional_recursion check would just become a single query invocation more expensive.

Last update: Sep 28 2020 at 14:45UTC