Stop using LLVM struct types for byval/sret #122050

erikdesjardins · 2024-03-06T00:07:03Z

For byval and sret, the type has no semantic meaning, only the size matters*†. Using [N x i8] is a more direct way to specify that we want N bytes, and avoids relying on LLVM's struct layout.

*: The alignment would matter, if we didn't explicitly specify it. From what I can tell, we always specified the alignment for sret; for byval, we didn't until #112157.

†: For byval, the hidden copy may be impacted by padding in the LLVM struct type, i.e. padding bytes may not be copied. (I'm not sure if this is done today, but I think it would be legal.) But we manually pad our LLVM struct types specifically to avoid there ever being LLVM-visible padding, so that shouldn't be an issue.

Split out from #121577.

r? @nikic

This avoids depending on LLVM's struct types to determine the size of the byval/sret slot.

erikdesjardins · 2024-03-06T00:07:44Z

tests/codegen/simd/unpadded-simd.rs

 // CHECK: %int16x4x2_t = type { <4 x i16>, <4 x i16> }
 #[no_mangle]
-fn takes_int16x4x2_t(t: int16x4x2_t) -> int16x4x2_t {
+extern "unadjusted" fn takes_int16x4x2_t(t: int16x4x2_t) -> int16x4x2_t {
    t
 }


This test only indirectly used the struct type via byval (so it would be removed by these changes), but the original motivation (#87254) was for the unadjusted ABI, where we use the struct type directly and pass the vectors by value. Changed it to test that.

the8472 · 2024-03-06T01:12:06Z

@bors try @rust-timer queue

Stop using LLVM struct types for byval/sret For `byval`, and `sret`, the type has no semantic meaning, only the size matters\*†. Using `[N x i8]` is a more direct way to specify that we want `N` bytes, and avoids relying on LLVM's struct layout. \*: The alignment would also matter if we didn't explicitly specify it. From what I can tell, we always specified the alignment for `sret`; for `byval`, we didn't until rust-lang#112157. †: For `byval`, the hidden copy may be impacted by padding in the LLVM struct type, i.e. padding bytes may not be copied. (I'm not sure if this is done today, but I think it would be legal.) But we manually pad our LLVM struct types specifically to avoid there ever being LLVM-visible padding, so that shouldn't be an issue. Split out from rust-lang#121577. r? `@nikic`

bors · 2024-03-06T01:13:56Z

⌛ Trying commit 96a7267 with merge d5b8881...

bors · 2024-03-06T02:42:24Z

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

bors · 2024-03-06T02:42:24Z

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

rust-timer · 2024-03-06T05:56:43Z

Finished benchmarking commit (d5b8881): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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
Improvements ✅ (primary)	-1.5%	[-2.4%, -0.3%]	3
Improvements ✅ (secondary)	-1.3%	[-1.3%, -1.3%]	1
All ❌✅ (primary)	-1.5%	[-2.4%, -0.3%]	3

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)	1.5%	[1.5%, 1.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[1.5%, 1.5%]	1

Cycles

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)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-3.0%	[-3.2%, -2.8%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.0%	[-3.2%, -2.8%]	2

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.2%	[-0.3%, -0.1%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.3%, -0.1%]	6

Bootstrap: 646.18s -> 643.479s (-0.42%)
Artifact size: 175.03 MiB -> 175.05 MiB (0.01%)

nikic · 2024-03-06T08:29:50Z

@bors r+ rollup=never

bors · 2024-03-06T08:29:53Z

📌 Commit 96a7267 has been approved by nikic

It is now in the queue for this repository.

RalfJung · 2024-03-07T07:00:41Z

compiler/rustc_codegen_llvm/src/abi.rs

Sounds like the docs at https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_llvm/abi/enum.PassMode.html#variant.Indirect should be updated then? Currently they say

This corresponds to the byval LLVM argument attribute (using the Rust type of this argument).

The parenthetical is no longer true with this patch.

Removed the parenthetical. The important part is that byval is used, the specific type is not so important (it could be [N x i8], iM, the Rust struct type, etc.)

RalfJung · 2024-03-07T15:15:50Z

I added that comment when trying to figure out where Rust types leak into the ABI, an effort that found a few subtle ABI bugs. So I think there can never be enough details in these ABI comments. In particular from the PR description it seems to be relevant that it has no padding? So i guess the pertinent information is: the the argument will be passed as one contiguous value on the stack (including all padding), with the alignment of the Rust type of the argument, and the offset determined by the system ABI.

erikdesjardins · 2024-03-07T23:03:13Z

Added more info.

Your point makes sense, especially as the alignment does not actually match the Rust type, nor is it guaranteed to be higher or lower than the Rust type's alignment. Instead, it's...complicated.

RalfJung · 2024-03-08T06:40:06Z

Oh wow.... and you're sure that's sound? Nothing is then relying on that alignment matching the Rust type alignment?

erikdesjardins · 2024-03-08T18:29:54Z

byval is only used for non-Rust ABIs, so doing this is necessary for soundness. If we use the Rust type's alignment, we'll read from an incorrect stack offset. (This is what caused #80127.)

Using a different alignment is fine in terms of Rust semantics because the byval pointer isn't usable from Rust code. (This is the same thing I touch on in this thread.)

~~If you do take a reference to such an argument, it gets copied to a higher-aligned alloca, which we have the freedom to do since it was passed by value.~~ Actually it doesn't (https://godbolt.org/z/cfM4PEGer), which is unsound. This is just a bug in the backend though--there's some code which skips the alloca if the source and destination have the same representation, and it must not be checking for alignment. (In other words, this isn't as bad as #112480--we can just fix it with no language-visible impact.) I'll open another PR to fix it.

We handle the reverse situation already--if you have a type where the Rust alignment is lower than the byval alignment, we copy it (https://godbolt.org/z/WYfj58o36) to a higher-aligned alloca before calling the byval function.

As for this PR, it doesn't change the status quo. Before this PR it would generate ptr byval(%HighAlign) align 4, and after ptr byval([32 x i8]) align 4, but of course both of those have the same alignment.

Edit: opened #122212

RalfJung · 2024-03-09T11:40:02Z

byval is only used for non-Rust ABIs, so doing this is necessary for soundness. If we use the Rust type's alignment, we'll read from an incorrect stack offset. (This is what caused #80127.)

I don't understand how that is possible. If the alignment differs between the Rust type and the C type, then things will go wrong in a bunch of places, not just for byval argument passing.

Or is it the case some some ABIs have entirely independent alignments for when an argument is "in memory" vs passed on the stack?

Unfortunately the PR links to "this comment" by @eddyb but the link is broken, so it's hard to read up on what happened. (EDIT: Ah, found it.) Anyway all that information should make it into suitable rustc comments as it'll be easier to find there.

Using a different alignment is fine in terms of Rust semantics because the byval pointer isn't usable from Rust code.

Ah, that's the key point. So I hope the codegen backend remembers this and never adds a Rust-type-based alignment annotation when working on these pointers.

@bors r- (to update the labels, this already left the queue when you pushed)

nikic · 2024-03-10T08:53:51Z

@bors r+

bors · 2024-03-10T08:53:53Z

📌 Commit 8fdd5e0 has been approved by nikic

It is now in the queue for this repository.

Stop using LLVM struct types for byval/sret For `byval` and `sret`, the type has no semantic meaning, only the size matters\*†. Using `[N x i8]` is a more direct way to specify that we want `N` bytes, and avoids relying on LLVM's struct layout. \*: The alignment would matter, if we didn't explicitly specify it. From what I can tell, we always specified the alignment for `sret`; for `byval`, we didn't until rust-lang#112157. †: For `byval`, the hidden copy may be impacted by padding in the LLVM struct type, i.e. padding bytes may not be copied. (I'm not sure if this is done today, but I think it would be legal.) But we manually pad our LLVM struct types specifically to avoid there ever being LLVM-visible padding, so that shouldn't be an issue. Split out from rust-lang#121577. r? `@nikic`

bors · 2024-03-10T18:27:49Z

⌛ Testing commit 8fdd5e0 with merge aba35a5...

bors · 2024-03-10T19:07:28Z

💔 Test failed - checks-actions

erikdesjardins · 2024-03-10T20:05:30Z

Ah, and of course if those tests never ran, or only ran on their target-specific builders, they wouldn't have ran on the x86 nopt builders either.

nikic · 2024-03-10T20:19:55Z

@bors r+

bors · 2024-03-10T20:19:58Z

📌 Commit f18c2f8 has been approved by nikic

It is now in the queue for this repository.

bors · 2024-03-11T04:45:31Z

⌛ Testing commit f18c2f8 with merge a6d93ac...

bors · 2024-03-11T06:44:44Z

☀️ Test successful - checks-actions
Approved by: nikic
Pushing a6d93ac to master...

rust-timer · 2024-03-11T09:11:51Z

Finished benchmarking commit (a6d93ac): 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
Regressions ❌ (secondary)	1.9%	[0.5%, 3.3%]	2
Improvements ✅ (primary)	-2.1%	[-2.4%, -1.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.4%, -1.9%]	2

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)	2.0%	[1.4%, 2.5%]	4
Regressions ❌ (secondary)	2.7%	[2.3%, 3.3%]	6
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[-2.7%, 2.5%]	5

Cycles

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)	2.0%	[1.8%, 2.1%]	5
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

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.2%	[-0.3%, -0.1%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.3%, -0.1%]	6

Bootstrap: 647.708s -> 645.581s (-0.33%)
Artifact size: 309.95 MiB -> 309.97 MiB (0.01%)

erikdesjardins · 2024-03-11T13:47:35Z

Those regressions (and probably improvements) are noise, they were undone in the next merge.

This is so direct that I feel justified to do

@rustbot label perf-regression-triaged

use [N x i8] for byval/sret types

96a7267

This avoids depending on LLVM's struct types to determine the size of the byval/sret slot.

rustbot assigned nikic Mar 6, 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 Mar 6, 2024

erikdesjardins commented Mar 6, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

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

erikdesjardins mentioned this pull request Mar 6, 2024

Stop using LLVM struct types for alloca #122053

Merged

This comment has been minimized.

Sign in to view

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

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 Mar 6, 2024

RalfJung reviewed Mar 7, 2024

View reviewed changes

fix now-incorrect parenthetical about byval attr

c56ffaa

erikdesjardins force-pushed the sret branch from 8b979e5 to c56ffaa Compare March 7, 2024 23:00

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 9, 2024

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 Mar 10, 2024

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 Mar 10, 2024

add -O to some tests which depend on attributes being added

f18c2f8

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 Mar 10, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 11, 2024

bors merged commit a6d93ac into rust-lang:master Mar 11, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 11, 2024

This was referenced Mar 11, 2024

Set writable and dead_on_unwind attributes for sret arguments #121298

Merged

Test wasm32-wasip1 in CI, not wasm32-unknown-unknown #122036

Merged

rustbot added the perf-regression Performance regression. label Mar 11, 2024

erikdesjardins deleted the sret branch March 11, 2024 13:19

nikic mentioned this pull request Mar 11, 2024

Copy byval argument to alloca if alignment is insufficient #122212

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 11, 2024

matthiaskrgr mentioned this pull request Sep 15, 2024

ICE: asked to assemble auto trait candidates of unexpected type: FreshTy(0) #130411

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using LLVM struct types for byval/sret #122050

Stop using LLVM struct types for byval/sret #122050

erikdesjardins commented Mar 6, 2024 •

edited

Loading

erikdesjardins Mar 6, 2024 •

edited

Loading

the8472 commented Mar 6, 2024

This comment has been minimized.

bors commented Mar 6, 2024

bors commented Mar 6, 2024

bors commented Mar 6, 2024

This comment has been minimized.

rust-timer commented Mar 6, 2024

nikic commented Mar 6, 2024

bors commented Mar 6, 2024

RalfJung Mar 7, 2024

erikdesjardins Mar 7, 2024

RalfJung commented Mar 7, 2024 via email

erikdesjardins commented Mar 7, 2024 •

edited

Loading

RalfJung commented Mar 8, 2024

erikdesjardins commented Mar 8, 2024 •

edited

Loading

RalfJung commented Mar 9, 2024 •

edited

Loading

nikic commented Mar 10, 2024

bors commented Mar 10, 2024

bors commented Mar 10, 2024

This comment has been minimized.

bors commented Mar 10, 2024

erikdesjardins commented Mar 10, 2024 •

edited

Loading

nikic commented Mar 10, 2024

bors commented Mar 10, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

rust-timer commented Mar 11, 2024

erikdesjardins commented Mar 11, 2024

Stop using LLVM struct types for byval/sret #122050

Stop using LLVM struct types for byval/sret #122050

Conversation

erikdesjardins commented Mar 6, 2024 • edited Loading

erikdesjardins Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

the8472 commented Mar 6, 2024

This comment has been minimized.

bors commented Mar 6, 2024

bors commented Mar 6, 2024

bors commented Mar 6, 2024

This comment has been minimized.

rust-timer commented Mar 6, 2024

Overall result: ✅ improvements - no action needed

nikic commented Mar 6, 2024

bors commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Mar 7, 2024 via email

erikdesjardins commented Mar 7, 2024 • edited Loading

RalfJung commented Mar 8, 2024

erikdesjardins commented Mar 8, 2024 • edited Loading

RalfJung commented Mar 9, 2024 • edited Loading

nikic commented Mar 10, 2024

bors commented Mar 10, 2024

bors commented Mar 10, 2024

This comment has been minimized.

bors commented Mar 10, 2024

erikdesjardins commented Mar 10, 2024 • edited Loading

nikic commented Mar 10, 2024

bors commented Mar 10, 2024

bors commented Mar 11, 2024

bors commented Mar 11, 2024

rust-timer commented Mar 11, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

erikdesjardins commented Mar 11, 2024

erikdesjardins commented Mar 6, 2024 •

edited

Loading

erikdesjardins Mar 6, 2024 •

edited

Loading

erikdesjardins commented Mar 7, 2024 •

edited

Loading

erikdesjardins commented Mar 8, 2024 •

edited

Loading

RalfJung commented Mar 9, 2024 •

edited

Loading

erikdesjardins commented Mar 10, 2024 •

edited

Loading