Allow `transmute`s to produce `OperandValue`s instead of needing `alloca`s #109843

scottmcm · 2023-04-01T19:24:38Z

LLVM can usually optimize these away, but especially for things like transmutes of newtypes it's silly to generate the alloc+store+load at all when it's actually a nop at LLVM level.

rustbot · 2023-04-01T19:24:43Z

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

scottmcm · 2023-04-01T21:27:30Z

Looks like I missed some ScalarPair cases
@rustbot author

scottmcm · 2023-04-01T22:59:32Z

Switched to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/traits/trait.BuilderMethods.html#tymethod.load_operand which handles all the cases and is better than what I was doing before anyway
@rustbot ready

WaffleLapkin

I'm not exactly sure what's happening here, but here are some initial thoughts

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

tests/codegen/intrinsics/transmute.rs

scottmcm · 2023-04-04T01:07:31Z

compiler/rustc_codegen_ssa/src/traits/type_.rs

+    /// For nearly all types this is the same as the [`backend_type`], however
+    /// `bool` (and other `0`-or-`1` values) are kept as [`BaseTypeMethods::type_i1`]
+    /// in registers but as [`BaseTypeMethods::type_i8`] in memory.


Your mention of uncertainty was a good prompt to write up some of what I learned as I was stumbling around trying to figure out how all this stuff worked 🙂

WaffleLapkin

Looks reasonable to me. r=me modulo the ZST question. (thanks for expanding docs!)

WaffleLapkin · 2023-04-04T14:13:18Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

-        let size_in_bytes = src.layout.size.bytes();
-        if size_in_bytes == 0 {
-            // Nothing to write


Is there a particular reason to remove the ZST check?

Because, as I dug more, it was really only useful for my manual memcpy; OperandValue::store already does it:

rust/compiler/rustc_codegen_ssa/src/mir/operand.rs

Lines 285 to 289 in be8e5ba

// Avoid generating stores of zero-sized values, because the only way to have a zero-sized

// value is through `undef`, and store itself is useless.

if dest.layout.is_zst() {

return;

}

WaffleLapkin · 2023-04-04T14:23:09Z

tests/codegen/simd-intrinsic/simd-intrinsic-transmute-array.rs

+    // CHECK: %[[VAL:.+]] = load <4 x float>, {{ptr %x|.+>\* %.+}}, align 4
+    // CHECK: store <4 x float> %[[VAL:.+]], {{ptr %0|.+>\* %.+}}, align 16
+    unsafe { std::mem::transmute(x) }


Don't think it's a problem, but it's interesting how S(x) produces a memcpy while transmute produces load+store, while doing identical work (I think?).

Quite! When I saw this I brought it up with the portable-simd folks, since it seems that LLVM doesn't normalize it in IR in either direction, so they might want to experiment with which is better.

scottmcm · 2023-04-04T18:26:02Z

@bors r=WaffleLapkin

bors · 2023-04-04T18:26:04Z

📌 Commit 7afaa7ce1861481fdb0af9a6469b5b60e47787c5 has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-04-04T22:12:09Z

⌛ Testing commit 7afaa7ce1861481fdb0af9a6469b5b60e47787c5 with merge 3913a2b4c2bbaf2dd5c5610ecfc6beea1ababd99...

bors · 2023-04-04T22:35:03Z

💔 Test failed - checks-actions

scottmcm · 2023-04-04T22:39:47Z

Of course, 32-bit catches me again 😭

@bors r-

bors · 2023-04-05T01:09:25Z

💔 Test failed - checks-actions

… `alloca`s LLVM can usually optimize these away, but especially for things like transmutes of newtypes it's silly to generate the `alloc`+`store`+`load` at all when it's actually a nop at LLVM level.

scottmcm · 2023-04-05T02:18:35Z

...and updated the test to not accidentally rely on standard library optimizations. Passed for -gnu-nopt in https://github.com/rust-lang/rust/actions/runs/4614082794/jobs/8156716838?pr=109843#step:26:1864

@bors r=WaffleLapkin rollup=iffy

bors · 2023-04-05T02:18:37Z

📌 Commit 9aa9a84 has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-04-05T03:26:42Z

⌛ Testing commit 9aa9a84 with merge 8d321f7...

bors · 2023-04-05T05:47:26Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 8d321f7 to master...

rust-timer · 2023-04-05T07:53:29Z

Finished benchmarking commit (8d321f7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-2.3%	[-3.6%, -1.2%]	3
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

Cycles

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

scottmcm · 2023-04-05T08:36:40Z

Wow, more transmutes out there than I'd thought -- 73 relevant binary size improvements in debug, with no (even non-relevant) size regressions.

(Nothing relevant for opt size, though, unsurprisingly.)

rustbot assigned WaffleLapkin Apr 1, 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 Apr 1, 2023

This comment has been minimized.

Sign in to view

scottmcm force-pushed the better-transmute branch from c49761b to 1e509af Compare April 1, 2023 20:54

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2023

This comment has been minimized.

Sign in to view

scottmcm force-pushed the better-transmute branch from 1e509af to 1aca038 Compare April 1, 2023 22:16

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 1, 2023

WaffleLapkin reviewed Apr 3, 2023

View reviewed changes

scottmcm force-pushed the better-transmute branch from 1aca038 to c3d1b2a Compare April 4, 2023 01:05

scottmcm commented Apr 4, 2023

View reviewed changes

scottmcm force-pushed the better-transmute branch from c3d1b2a to 7afaa7c Compare April 4, 2023 01:21

WaffleLapkin approved these changes Apr 4, 2023

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 Apr 4, 2023

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 Apr 4, 2023

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 Apr 5, 2023

This comment has been minimized.

Sign in to view

Allow transmutes to produce OperandValues instead of always using…

9aa9a84

… `alloca`s LLVM can usually optimize these away, but especially for things like transmutes of newtypes it's silly to generate the `alloc`+`store`+`load` at all when it's actually a nop at LLVM level.

scottmcm force-pushed the better-transmute branch from 462652d to 9aa9a84 Compare April 5, 2023 01:44

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Apr 5, 2023

scottmcm force-pushed the better-transmute branch from 53ac230 to 9aa9a84 Compare April 5, 2023 02:16

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 5, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 5, 2023

bors merged commit 8d321f7 into rust-lang:master Apr 5, 2023

rustbot added this to the 1.70.0 milestone Apr 5, 2023

scottmcm deleted the better-transmute branch April 5, 2023 05:48

scottmcm mentioned this pull request Apr 5, 2023

Missing optimization when transmuting enums #109958

Closed

matthiaskrgr mentioned this pull request Apr 6, 2023

LLVM_ERROR: invalid bitcast: transmute #110005

Closed

ehuss mentioned this pull request Apr 14, 2023

Spurious CI failure: tests/run-make/jobserver-error #110321

Open

scottmcm mentioned this pull request Mar 12, 2024

Lower transmutes from int to pointer type as gep on null #121282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `transmute`s to produce `OperandValue`s instead of needing `alloca`s #109843

Allow `transmute`s to produce `OperandValue`s instead of needing `alloca`s #109843

scottmcm commented Apr 1, 2023

rustbot commented Apr 1, 2023

This comment has been minimized.

scottmcm commented Apr 1, 2023

This comment has been minimized.

scottmcm commented Apr 1, 2023

WaffleLapkin left a comment

scottmcm Apr 4, 2023

WaffleLapkin left a comment

WaffleLapkin Apr 4, 2023

scottmcm Apr 4, 2023 •

edited

Loading

WaffleLapkin Apr 4, 2023

scottmcm Apr 4, 2023

scottmcm commented Apr 4, 2023

bors commented Apr 4, 2023

bors commented Apr 4, 2023

This comment has been minimized.

bors commented Apr 4, 2023

scottmcm commented Apr 4, 2023

bors commented Apr 5, 2023

This comment has been minimized.

scottmcm commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 5, 2023

rust-timer commented Apr 5, 2023

scottmcm commented Apr 5, 2023

	// Avoid generating stores of zero-sized values, because the only way to have a zero-sized
	// value is through `undef`, and store itself is useless.
	if dest.layout.is_zst() {
	return;
	}

Allow transmutes to produce OperandValues instead of needing allocas #109843

Allow transmutes to produce OperandValues instead of needing allocas #109843

Conversation

scottmcm commented Apr 1, 2023

rustbot commented Apr 1, 2023

This comment has been minimized.

scottmcm commented Apr 1, 2023

This comment has been minimized.

scottmcm commented Apr 1, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

scottmcm Apr 4, 2023

Choose a reason for hiding this comment

WaffleLapkin left a comment

Choose a reason for hiding this comment

WaffleLapkin Apr 4, 2023

Choose a reason for hiding this comment

scottmcm Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

WaffleLapkin Apr 4, 2023

Choose a reason for hiding this comment

scottmcm Apr 4, 2023

Choose a reason for hiding this comment

scottmcm commented Apr 4, 2023

bors commented Apr 4, 2023

bors commented Apr 4, 2023

This comment has been minimized.

bors commented Apr 4, 2023

scottmcm commented Apr 4, 2023

bors commented Apr 5, 2023

This comment has been minimized.

scottmcm commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 5, 2023

rust-timer commented Apr 5, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

scottmcm commented Apr 5, 2023

Allow `transmute`s to produce `OperandValue`s instead of needing `alloca`s #109843

Allow `transmute`s to produce `OperandValue`s instead of needing `alloca`s #109843

scottmcm Apr 4, 2023 •

edited

Loading