rustc_codegen_ssa: only create backend `BasicBlock`s as-needed. #84993

eddyb · 2021-05-06T16:05:35Z

Instead of creating one backend (e.g. LLVM) block per MIR block ahead of time, and then deleting the ones that weren't visited, this PR moves to creating the blocks as they're needed (either reached via the RPO visit, or used as the target of a branch from a different block).

As deleting a block was the only unsafe builder method (generally we only create backend objects, not remove them), that's gone now and codegen is overall a bit safer.

The only change in output is the order of LLVM blocks (which AFAIK has no semantic meaning, other than the first block being the entry block). This happens because the blocks are now created due to control-flow edges, rather than MIR block order.

I'm making this a standalone PR because I keep getting wild perf results when I change anything in codegen, but if you want to read more about my plans in this area, see #84771 (comment) (and #84771 (comment) - but that may be a bit outdated).

(You may notice some of the APIs in this PR, like append_block, don't help with the future plans - but I didn't want to include the necessary refactors that pass a build around everywhere, in this PR, so it's a small compromise)

r? @nagisa @bjorn3

eddyb · 2021-05-06T16:05:44Z

@bors try @rust-timer queue

rust-timer · 2021-05-06T16:05:46Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-05-06T16:05:53Z

⌛ Trying commit 1557955334c9ba1145b41cbfe0bec0f9dbfa9e97 with merge 2c3602faa8930c104e00dc0b9d0e2ce5246f1caf...

bors · 2021-05-06T17:06:21Z

☀️ Try build successful - checks-actions
Build commit: 2c3602faa8930c104e00dc0b9d0e2ce5246f1caf (2c3602faa8930c104e00dc0b9d0e2ce5246f1caf)

rust-timer · 2021-05-06T17:06:23Z

Queued 2c3602faa8930c104e00dc0b9d0e2ce5246f1caf with parent 109248a, future comparison URL.

rust-timer · 2021-05-06T23:38:15Z

Finished benchmarking try commit (2c3602faa8930c104e00dc0b9d0e2ce5246f1caf): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

eddyb · 2021-05-07T00:09:09Z

I'm not sure I understand why there's so much... noise(?).

One thing that worries me is e.g. match-stress-enum-check having a consistent 1.1% between full and incr-full. And both in check_match.

Is it possible changing the block order causes pseudorandom effects in LLVM quality, not just performance, i.e. causing parts of rustc to be optimized differently?

Maybe it would be useful to be able to do a "stage1 perf run" (to see if it doesn't vary as much as stage2 does) but it's harder because of proc macros (you'd need to use the right beta to compile them).

nagisa

This looks reasonable enough on the surface, and the timings appear to be neutral.

I wondered if coverage depends on all of the unreachable blocks ending up in the IR (as otherwise uncovered regions wouldn't show up), but if the coverage tests pass, this seems LGTM.

r=me in that case.

nagisa · 2021-05-06T23:58:07Z

src/test/codegen/match.rs

 // CHECK: [[OTHERWISE]]:
 // CHECK-NEXT: unreachable
 // CHECK: [[A]]:
 // CHECK-NEXT: store i8 0, i8* %1, align 1
+// CHECK-NEXT: br label %[[EXIT:[a-zA-Z0-9_]+]]
+// CHECK: [[B]]:


This suggests to me these checks want to be CHECK-DAG but sounds like it'd be a major PiTA to adjust.

There's a CHECK-DAG feature? I might look into it, it's just these two tests.

nagisa · 2021-05-07T00:12:02Z

compiler/rustc_codegen_ssa/src/mir/block.rs

+    // more backend-agnostic prefix such as `cg` (i.e. this would be `cgbb`).
+    pub fn llbb(&mut self, bb: mir::BasicBlock) -> Bx::BasicBlock {
+        self.cached_llbbs[bb].unwrap_or_else(|| {
+            // FIXME(eddyb) only name the block if `fewer_names` is `false`.


Would it not make sense to just check for this within append_block function? I guess if we did that, we'd end up in a situation where we still format! potentially many strings only for them to be ignored?

Yeah - I think I want to solve it similar to how I did for SSA values and set_name on them, I just don't want to do it now.

nagisa · 2021-05-07T00:14:51Z

FWIW I strongly doubt you can get deterministic timings when the code received by LLVM changes in more significant ways but otherwise don't really have much impact on how heavy it is, such as is this PR.

eddyb · 2021-05-07T00:16:18Z

I wondered if coverage depends on all of the unreachable blocks ending up in the IR

We were deleting unreachable blocks, so if coverage depended on them, it wouldn't have worked.

I believe this PR shouldn't change which blocks exist after codege, only what order they're in.

eddyb · 2021-05-07T00:25:02Z

Oh just saw the "r=me", thanks!

@bors r=nagisa

bors · 2021-05-07T00:25:03Z

📌 Commit 1557955334c9ba1145b41cbfe0bec0f9dbfa9e97 has been approved by nagisa

bors · 2021-05-07T03:57:21Z

⌛ Testing commit 1557955334c9ba1145b41cbfe0bec0f9dbfa9e97 with merge a8c109b2e4bb2469d262699d2be9202c3e2ee245...

bors · 2021-05-07T04:35:32Z

💔 Test failed - checks-actions

eddyb · 2021-05-07T04:43:54Z

I was worried about this - I'll either have to use CHECK-DAG (seems potentially hard) or do another step of the refactors: that is, inject the br to the actual block, from the cleanup_pad block, after building the blocks corresponding to MIR blocks.

eddyb · 2021-05-15T11:08:38Z

Not expecting anything interesting, maybe more random noise, but might as well:
@bors try @rust-timer queue

rust-timer · 2021-05-15T11:08:40Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-05-15T11:08:47Z

⌛ Trying commit edf90cdeffa49f5787df82790a63b8f687e991a2 with merge efa2875e004f7119f732474853a41e8d4eff22b6...

bors · 2021-05-15T11:53:53Z

☀️ Try build successful - checks-actions
Build commit: efa2875e004f7119f732474853a41e8d4eff22b6 (efa2875e004f7119f732474853a41e8d4eff22b6)

rust-timer · 2021-05-15T11:53:55Z

Queued efa2875e004f7119f732474853a41e8d4eff22b6 with parent c6dd87a, future comparison URL.

rust-timer · 2021-05-15T16:21:32Z

Finished benchmarking try commit (efa2875e004f7119f732474853a41e8d4eff22b6): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

eddyb · 2021-05-15T18:46:43Z

Doesn't seem like a lot of change from #84993 (comment) - which makes sense, we're still only testing block order with GNU-style EH.

…nagisa rustc_codegen_ssa: generate MSVC cleanup pads on demand, like GNU landing pads. This unblocks rust-lang#84993 in terms of codegen tests, as it brings the MSVC-style (`cleanup_pad`) EH (LLVM) block order in line with the GNU-style (`landing_pad`) EH (LLVM) block order, by having both of them be on-demand (instead of MSVC-style being eager and GNU-style lazy/on-demand). It also unifies the two implementations a bit, similar to rust-lang#84699, but in the opposite direction (as that attempt made both kinds of EH pads eagerly built). ~~Opening as draft because I haven't done enough Windows testing just yet, of both this PR, and of rust-lang#84993 rebased on it.~~ (**EDIT**: seems to be working as expected) r? `@nagisa`

eddyb · 2021-05-16T22:11:54Z

So hopefully that's enough time for this to not end up in the same nightly as #85316:

@bors r=nagisa

bors · 2021-05-16T22:11:56Z

📌 Commit 0fcaf11 has been approved by nagisa

klensy · 2021-05-16T22:31:42Z

So hopefully that's enough time for this to not end up in the same nightly as #85316:

Current (at least rustup says that) nightly is rustc 1.54.0-nightly (8cf990c 2021-05-15), so they can get in one build.

bors · 2021-05-16T23:00:58Z

⌛ Testing commit 0fcaf11 with merge a55748f...

bors · 2021-05-17T01:41:40Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing a55748f to master...

eddyb · 2021-05-17T04:56:39Z

@klensy Yeah but the trick is that this PR took 2h41m to build, and started one hour (AFAIK) before the nightly was published, so it couldn't have gotten in. Either way, it worked:

info: latest update on 2021-05-17, rust version 1.54.0-nightly (fe72845 2021-05-16)

That contains #85316 but not this PR.

rust-highfive assigned nagisa May 6, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 6, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 6, 2021

nagisa reviewed May 7, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2021

eddyb mentioned this pull request May 7, 2021

rustc_codegen_ssa: turn builders "unpositioned" after emitting a terminator. #84771

Closed

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 7, 2021

eddyb force-pushed the cg-ssa-on-demand-blocks branch from 1557955 to d010d70 Compare May 15, 2021 03:54

eddyb mentioned this pull request May 15, 2021

rustc_codegen_ssa: generate MSVC cleanup pads on demand, like GNU landing pads. #85316

Merged

eddyb force-pushed the cg-ssa-on-demand-blocks branch from d010d70 to edf90cd Compare May 15, 2021 08:10

eddyb marked this pull request as draft May 15, 2021 08:10

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 15, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 15, 2021

eddyb marked this pull request as ready for review May 16, 2021 21:01

eddyb added 2 commits May 17, 2021 00:04

rustc_codegen_ssa: only create backend BasicBlocks as-needed.

402e9ef

rustc_codegen_ssa: append blocks to functions w/o creating a builder.

0fcaf11

eddyb force-pushed the cg-ssa-on-demand-blocks branch from edf90cd to 0fcaf11 Compare May 16, 2021 21:04

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 17, 2021

bors merged commit a55748f into rust-lang:master May 17, 2021

rustbot added this to the 1.54.0 milestone May 17, 2021

eddyb deleted the cg-ssa-on-demand-blocks branch May 17, 2021 04:56

eddyb mentioned this pull request May 24, 2021

rustup: update to nightly-2021-05-24. EmbarkStudios/rust-gpu#631

Merged

pnkfelix mentioned this pull request Jul 15, 2021

match an std::cmp::Ordering generates less optimized code in nightly #86511

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_codegen_ssa: only create backend `BasicBlock`s as-needed. #84993

rustc_codegen_ssa: only create backend `BasicBlock`s as-needed. #84993

eddyb commented May 6, 2021

eddyb commented May 6, 2021

rust-timer commented May 6, 2021

bors commented May 6, 2021

bors commented May 6, 2021

rust-timer commented May 6, 2021

rust-timer commented May 6, 2021

eddyb commented May 7, 2021

nagisa left a comment

nagisa May 6, 2021

eddyb May 7, 2021

nagisa May 7, 2021

eddyb May 7, 2021

nagisa commented May 7, 2021

eddyb commented May 7, 2021

eddyb commented May 7, 2021

bors commented May 7, 2021

bors commented May 7, 2021

This comment has been minimized.

bors commented May 7, 2021

eddyb commented May 7, 2021

eddyb commented May 15, 2021

rust-timer commented May 15, 2021

bors commented May 15, 2021

bors commented May 15, 2021

rust-timer commented May 15, 2021

rust-timer commented May 15, 2021

eddyb commented May 15, 2021

eddyb commented May 16, 2021

bors commented May 16, 2021

klensy commented May 16, 2021

bors commented May 16, 2021

bors commented May 17, 2021

eddyb commented May 17, 2021 •

edited

Loading

rustc_codegen_ssa: only create backend BasicBlocks as-needed. #84993

rustc_codegen_ssa: only create backend BasicBlocks as-needed. #84993

Conversation

eddyb commented May 6, 2021

eddyb commented May 6, 2021

rust-timer commented May 6, 2021

bors commented May 6, 2021

bors commented May 6, 2021

rust-timer commented May 6, 2021

rust-timer commented May 6, 2021

eddyb commented May 7, 2021

nagisa left a comment

Choose a reason for hiding this comment

nagisa May 6, 2021

Choose a reason for hiding this comment

eddyb May 7, 2021

Choose a reason for hiding this comment

nagisa May 7, 2021

Choose a reason for hiding this comment

eddyb May 7, 2021

Choose a reason for hiding this comment

nagisa commented May 7, 2021

eddyb commented May 7, 2021

eddyb commented May 7, 2021

bors commented May 7, 2021

bors commented May 7, 2021

This comment has been minimized.

bors commented May 7, 2021

eddyb commented May 7, 2021

eddyb commented May 15, 2021

rust-timer commented May 15, 2021

bors commented May 15, 2021

bors commented May 15, 2021

rust-timer commented May 15, 2021

rust-timer commented May 15, 2021

eddyb commented May 15, 2021

eddyb commented May 16, 2021

bors commented May 16, 2021

klensy commented May 16, 2021

bors commented May 16, 2021

bors commented May 17, 2021

eddyb commented May 17, 2021 • edited Loading

rustc_codegen_ssa: only create backend `BasicBlock`s as-needed. #84993

rustc_codegen_ssa: only create backend `BasicBlock`s as-needed. #84993

eddyb commented May 17, 2021 •

edited

Loading