`codegen_ssa` cleanups #113879

nnethercote · 2023-07-20T05:11:01Z

Some clarifications I made when reading this code closely.

r? @tmiasko

compiler/rustc_codegen_ssa/src/back/write.rs

rustbot · 2023-07-21T03:05:53Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

nnethercote · 2023-07-21T03:06:17Z

I added some more commits. Best reviewed one commit at a time.

nnethercote · 2023-07-23T22:37:38Z

I added one more small commit. That should be the end of them :)

compiler/rustc_codegen_ssa/src/base.rs

compiler/rustc_codegen_ssa/src/back/write.rs

bors · 2023-07-30T22:41:30Z

☔ The latest upstream changes (presumably #114264) made this pull request unmergeable. Please resolve the merge conflicts.

It has a single callsite, and provides little value.

And rename the `Compiled` variant as `Finished`, because that name makes it clearer there is nothing left to do, contrasting nicely with the `Needs*` variants.

- Thin and fat LTO can't happen together. - `NeedsLink` and (non-allocator) `Compiled` work item results can't happen together.

It took me some time to understand how the main thread can lend a jobserver token to an LLVM thread. This commit renames a couple of things to make it clearer. - Rename the `LLVMing` variant as `Lending`, because that is a clearer description of what is happening. - Rename `running` as `running_with_own_token`, which makes it clearer that there might be one additional LLVM thread running (with a loaned token). Also add a comment to its definition.

The `Worker` is unnecessary, and just makes it longer than necessary.

This is useful when profiling with a profiler like Samply.

It's no longer used, and `spawn_named_thread` is preferable, because naming threads is helpful when profiling.

Make it match the corresponding comment at the start of the unstable options.

Because it's usefulness wasn't clear to me, and I initially wondered if it could be removed. The text is based on the text in rust-lang#50972, the PR that added the flag.

`CodegenContext` is immutable except for the `worker` field - we clone `CodegenContext` in multiple places, changing the `worker` field each time. It's simpler to move the `worker` field out of `CodegenContext`.

The two functions are alway called together. This commit factors out the repeated code.

This loop condition involves `codegen_state`, `work_items`, and `running_with_own_token`. But the body of the loop cannot modify `codegen_state`, so repeatedly checking it is unnecessary.

The main loop has a *very* complex condition, which includes two mentions of `codegen_state`. The body of the loop then immediately switches on the `codegen_state`. I find it easier to understand if it's a `loop` and we check for exit conditions after switching on `codegen_state`. We end up with a tiny bit of code duplication, but it's clear that (a) we never exit in the `Ongoing` case, (b) we exit in the `Completed` state only if several things are true (and there's interaction with LTO there), and (c) we exit in the `Aborted` state if a couple of things are true. Also, the exit conditions are all simple conjunctions.

It makes things a little clearer.

PR rust-lang#112946 tweaked the naming of LLVM threads, but messed things up slightly, resulting in threads on Windows having names like `optimize module {} regex.f10ba03eb5ec7975-cgu.0`. This commit removes the extraneous `{} `.

This function has some shared code for the thin LTO and fat LTO cases, but those cases have so little in common that it's actually clearer to treat them fully separately.

nnethercote · 2023-07-31T06:38:25Z

I have rebased.

bjorn3 · 2023-07-31T07:47:41Z

@bors r+

bors · 2023-07-31T07:47:43Z

📌 Commit c17c8dc has been approved by bjorn3

It is now in the queue for this repository.

bors · 2023-07-31T08:18:23Z

⌛ Testing commit c17c8dc with merge 5082281...

bors · 2023-07-31T10:08:03Z

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 5082281 to master...

rust-timer · 2023-07-31T12:31:05Z

Finished benchmarking commit (5082281): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 652.303s -> 652.574s (0.04%)

rustbot assigned tmiasko Jul 20, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2023

lqd reviewed Jul 20, 2023

View reviewed changes

compiler/rustc_codegen_ssa/src/back/write.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the codegen_ssa-cleanups branch from d8b1635 to bbc03e2 Compare July 20, 2023 09:00

nnethercote force-pushed the codegen_ssa-cleanups branch from 8fbd1a9 to f7eb0a4 Compare July 23, 2023 22:33

nnethercote mentioned this pull request Jul 23, 2023

Two codegen fixes #113775

Closed

bjorn3 reviewed Jul 29, 2023

View reviewed changes

compiler/rustc_codegen_ssa/src/base.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jul 29, 2023

View reviewed changes

compiler/rustc_codegen_ssa/src/back/write.rs Show resolved Hide resolved

nnethercote added 17 commits July 31, 2023 16:20

Inline and remove submit_pre_codegened_module_to_llvm.

a8c71f0

It has a single callsite, and provides little value.

Add comments to WorkItemResult.

4f598b8

And rename the `Compiled` variant as `Finished`, because that name makes it clearer there is nothing left to do, contrasting nicely with the `Needs*` variants.

Add some assertions.

fd017d3

- Thin and fat LTO can't happen together. - `NeedsLink` and (non-allocator) `Compiled` work item results can't happen together.

Rename MainThreadWorkerState.

f81fe9d

The `Worker` is unnecessary, and just makes it longer than necessary.

Remove an unnecessary pub.

8b9e3f0

Remove some unused values in codegen_crate.

176610c

Give the coordinator thread a name.

e78fb95

This is useful when profiling with a profiler like Samply.

Remove ExtraBackendMethods::spawn_thread.

4a120f3

It's no longer used, and `spawn_named_thread` is preferable, because naming threads is helpful when profiling.

Fix a comment.

67e4bec

Make it match the corresponding comment at the start of the unstable options.

Document -Zno-parallel-llvm.

0ea9950

Because it's usefulness wasn't clear to me, and I initially wondered if it could be removed. The text is based on the text in rust-lang#50972, the PR that added the flag.

Remove CodegenContext::worker.

3517fe8

`CodegenContext` is immutable except for the `worker` field - we clone `CodegenContext` in multiple places, changing the `worker` field each time. It's simpler to move the `worker` field out of `CodegenContext`.

Move maybe_start_llvm_timer's body into spawn_work.

d21d31c

The two functions are alway called together. This commit factors out the repeated code.

Tweak a loop condition.

179bf19

This loop condition involves `codegen_state`, `work_items`, and `running_with_own_token`. But the body of the loop cannot modify `codegen_state`, so repeatedly checking it is unnecessary.

Use standard Rust capitalization rules for names containing "LTO".

3b44f5b

Introduce running_with_any_token closure.

90ce358

It makes things a little clearer.

nnethercote added 3 commits July 31, 2023 16:21

Fix LLVM thread names on Windows.

d404699

PR rust-lang#112946 tweaked the naming of LLVM threads, but messed things up slightly, resulting in threads on Windows having names like `optimize module {} regex.f10ba03eb5ec7975-cgu.0`. This commit removes the extraneous `{} `.

Clean up generate_lto_work.

5673f47

This function has some shared code for the thin LTO and fat LTO cases, but those cases have so little in common that it's actually clearer to treat them fully separately.

Remove unnecessary semicolon.

c17c8dc

nnethercote force-pushed the codegen_ssa-cleanups branch from 797e703 to c17c8dc Compare July 31, 2023 06:37

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 Jul 31, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 31, 2023

bors merged commit 5082281 into rust-lang:master Jul 31, 2023

rustbot added this to the 1.73.0 milestone Jul 31, 2023

nnethercote deleted the codegen_ssa-cleanups branch August 1, 2023 04:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`codegen_ssa` cleanups #113879

`codegen_ssa` cleanups #113879

nnethercote commented Jul 20, 2023

rustbot commented Jul 21, 2023

nnethercote commented Jul 21, 2023

nnethercote commented Jul 23, 2023

bors commented Jul 30, 2023

nnethercote commented Jul 31, 2023

bjorn3 commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

rust-timer commented Jul 31, 2023

codegen_ssa cleanups #113879

codegen_ssa cleanups #113879

Conversation

nnethercote commented Jul 20, 2023

rustbot commented Jul 21, 2023

nnethercote commented Jul 21, 2023

nnethercote commented Jul 23, 2023

bors commented Jul 30, 2023

nnethercote commented Jul 31, 2023

bjorn3 commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

rust-timer commented Jul 31, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

`codegen_ssa` cleanups #113879

`codegen_ssa` cleanups #113879