Avoid `alloca`s in codegen for simple `mir::Aggregate` statements #123886

scottmcm · 2024-04-13T04:22:09Z

The core idea here is to remove the abstraction penalty of simple newtypes in codegen.

Even something simple like constructing a

#[repr(transparent)] struct Foo(u32);

forces an alloca to be generated in nightly right now.

Certainly LLVM can optimize that away, but it would be nice if it didn't have to.

Quick example:

#[repr(transparent)]
pub struct Transparent32(u32);

#[no_mangle]
pub fn make_transparent(x: u32) -> Transparent32 {
    let a = Transparent32(x);
    a
}

on nightly we produce https://rust.godbolt.org/z/zcvoM79ae

define noundef i32 @make_transparent(i32 noundef %x) unnamed_addr #0 {
  %a = alloca i32, align 4
  store i32 %x, ptr %a, align 4
  %0 = load i32, ptr %a, align 4, !noundef !3
  ret i32 %0
}

but after this PR we produce

define noundef i32 @make_transparent(i32 noundef %x) unnamed_addr #0 {
start:
  ret i32 %x
}

(even before the optimizer runs).

rustbot · 2024-04-13T04:22:16Z

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

scottmcm · 2024-04-13T04:24:02Z

@bors try @rust-timer queue

Avoid `alloca`s in codegen for simple pairs and simple transparent structs Even something simple like constructing a ```rust #[repr(transparent)] struct Foo(u32); ``` forces an `alloca` to be generated in nightly right now. Certainly LLVM can optimize that away, but it would be nice if it didn't have to. Quick example: ```rust #[repr(transparent)] pub struct Transparent32(u32); #[no_mangle] pub fn make_transparent(x: u32) -> Transparent32 { let a = Transparent32(x); a } ``` on nightly we produce <https://rust.godbolt.org/z/zcvoM79ae> ```llvm define noundef i32 `@make_transparent(i32` noundef %x) unnamed_addr #0 { %a = alloca i32, align 4 store i32 %x, ptr %a, align 4 %0 = load i32, ptr %a, align 4, !noundef !3 ret i32 %0 } ``` but after this PR we produce ```llvm define noundef i32 `@make_transparent(i32` noundef %x) unnamed_addr #0 { start: ret i32 %x } ``` (even before the optimizer runs).

bors · 2024-04-13T04:25:13Z

⌛ Trying commit dd8ffd0 with merge 8dfd58a...

scottmcm · 2024-04-13T06:34:57Z

@bors try @rust-timer queue

bors · 2024-04-13T06:36:07Z

⌛ Trying commit 7cb89e3 with merge 1d0d3d3...

Avoid `alloca`s in codegen for simple pairs and simple transparent structs Even something simple like constructing a ```rust #[repr(transparent)] struct Foo(u32); ``` forces an `alloca` to be generated in nightly right now. Certainly LLVM can optimize that away, but it would be nice if it didn't have to. Quick example: ```rust #[repr(transparent)] pub struct Transparent32(u32); #[no_mangle] pub fn make_transparent(x: u32) -> Transparent32 { let a = Transparent32(x); a } ``` on nightly we produce <https://rust.godbolt.org/z/zcvoM79ae> ```llvm define noundef i32 `@make_transparent(i32` noundef %x) unnamed_addr #0 { %a = alloca i32, align 4 store i32 %x, ptr %a, align 4 %0 = load i32, ptr %a, align 4, !noundef !3 ret i32 %0 } ``` but after this PR we produce ```llvm define noundef i32 `@make_transparent(i32` noundef %x) unnamed_addr #0 { start: ret i32 %x } ``` (even before the optimizer runs).

fee1-dead · 2024-04-13T06:40:52Z

r? compiler

bors · 2024-04-13T08:09:10Z

☀️ Try build successful - checks-actions
Build commit: 1d0d3d3 (1d0d3d3e219daac686e2fd9110e09772b6d18dfc)

klensy · 2024-04-13T08:09:34Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

                // According to `rvalue_creates_operand`, only ZST
-                // aggregate rvalues are allowed to be operands.
+                // repat rvalues are allowed to be operands.


klensy · 2024-04-13T08:27:40Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

+                let ty = self.monomorphize(ty);
+                let layout = self.cx.layout_of(self.monomorphize(ty));


Is second call to monomorphize required?

lqd · 2024-04-13T12:21:37Z

(I've seen regex and wg-grammar being noisy in other PRs)

scottmcm · 2024-04-14T00:52:43Z

@bors try @rust-timer queue

scottmcm · 2024-05-09T05:23:40Z

@matthewjasper just wanted to make sure you have a notification for this review since the r? might not have sent you one.

matthewjasper · 2024-05-10T17:02:01Z

@bors r+

bors · 2024-05-10T17:02:03Z

📌 Commit c38f75c has been approved by matthewjasper

It is now in the queue for this repository.

bors · 2024-05-10T20:17:25Z

⌛ Testing commit c38f75c with merge 6e1d947...

bors · 2024-05-10T22:23:46Z

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing 6e1d947 to master...

rust-timer · 2024-05-11T00:22:24Z

Finished benchmarking commit (6e1d947): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%	[0.2%, 0.3%]	6
Regressions ❌ (secondary)	0.5%	[0.2%, 1.3%]	4
Improvements ✅ (primary)	-0.6%	[-1.0%, -0.3%]	6
Improvements ✅ (secondary)	-0.7%	[-1.1%, -0.4%]	2
All ❌✅ (primary)	-0.2%	[-1.0%, 0.3%]	12

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.4%	[3.4%, 3.4%]	1
Regressions ❌ (secondary)	4.8%	[4.8%, 4.8%]	1
Improvements ✅ (primary)	-1.8%	[-3.0%, -0.6%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-3.0%, 3.4%]	5

Cycles

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

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.6%, -0.0%]	73
Improvements ✅ (secondary)	-0.5%	[-1.6%, -0.0%]	11
All ❌✅ (primary)	-0.7%	[-1.6%, -0.0%]	73

Bootstrap: 673.906s -> 673.586s (-0.05%)
Artifact size: 315.77 MiB -> 315.84 MiB (0.02%)

nnethercote · 2024-05-11T02:07:33Z

Nice binary size reductions. They are almost entirely in debug builds, indicating that "Certainly LLVM can optimize that away" is correct but that only happens in opt builds :)

scottmcm · 2024-05-11T05:20:04Z

Yeah; I was hoping that LLVM not needing to optimize them away would be more of a perf win in opt, but I'll take instruction-neutral improvements to debug build perf too :)

aapoalas · 2024-05-11T05:20:17Z

tests/codegen/mir-aggregate-no-alloca.rs

+    std::cell::Cell::new(b)
+}
+
+// CHECK-LABLE: { i8, i16 } @make_cell_of_bool_and_short(i1 noundef zeroext %b, i16 noundef %s)


Typo here: "CHECK LABLE" should be "LABEL". The test is presumably not working properly.

Good eye; thank you. I've added a fix for that to #124999

fix few typos in filecheck annotations Inspired by rust-lang#123886 (comment) `rg -g '*.rs' '//\s+?[\w-]+(-[\w]+):' -r '$1' -oNI | sort -u` Should https://llvm.org/docs/CommandGuide/FileCheck.html#cmdoption-FileCheck-ignore-case be used for case-insensetive match for filecheck?

fix few typos in filecheck annotations Inspired by rust-lang/rust#123886 (comment) `rg -g '*.rs' '//\s+?[\w-]+(-[\w]+):' -r '$1' -oNI | sort -u` Should https://llvm.org/docs/CommandGuide/FileCheck.html#cmdoption-FileCheck-ignore-case be used for case-insensetive match for filecheck?

Unify `Rvalue::Aggregate` paths in cg_ssa In rust-lang#123840 and rust-lang#123886 I added two different codepaths for `Rvalue::Aggregate` in `cg_ssa`. This merges them into one, since raw pointers are also immediates that can be built from the immediates of their "fields".

Kobzol · 2024-05-14T08:40:40Z

Nice binary size reductions. The regressions are limited to a single benchmark (bitmaps), while the wins are across three different (primary) benchmarks.

@rustbot label: +perf-regression-triaged

rustbot assigned fee1-dead Apr 13, 2024

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 Apr 13, 2024