Add ability to transmute (somewhat) with generic consts in arrays #106281

JulianKnodt · 2022-12-30T02:19:58Z

Previously if the expression contained generic consts and did not have a directly equivalent type, transmuting the type in this way was forbidden, despite the two sizes being identical. Instead, we should be able to lazily tell if the two consts are identical, and if so allow them to be transmuted.

This is done by normalizing the forms of expressions into sorted order of multiplied terms, which is not generic over all expressions, but should handle most cases.

This allows for some basic transmutations between types that are equivalent in size without requiring additional stack space at runtime.

I only see one other location at which SizeSkeleton is being used, and it checks for equality so this shouldn't affect anywhere else that I can tell.

See this Stackoverflow post for what was previously necessary to convert between types. This PR makes converting nested T -> [T; 1] transmutes possible, and [uB*2; N] -> [uB; N * 2] possible as well.

I'm not sure whether this is something that would be wanted, and if it is it definitely should not be insta-stable, so I'd add a feature gate.

rustbot · 2022-12-30T02:20:04Z

r? @nagisa

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

src/test/ui/const-generics/transmute.rs

nagisa · 2023-01-28T20:52:33Z

cc @eddyb may have some thoughts on the changes to the layout code.

eddyb

Overall the approach looks sound (if compiler/lang teams are fine with it etc.), left some comments, don't really have much to say other than nitpicking though.

compiler/rustc_middle/src/ty/layout.rs

eddyb · 2023-01-28T21:21:31Z

compiler/rustc_middle/src/ty/layout.rs

+            // constants are always pre-normalized into a canonical form so this
+            // only needs to check if their pointers are identical.
+            (SizeSkeleton::Generic(a), SizeSkeleton::Generic(b)) => a == b,


This doesn't need to be cheap, we could plausibly run a SAT/SMT solver here (since this is for "proving" two types have the same size). Feels weird to canonicalize ahead of time when ty::Const itself doesn't do it, but I don't know much about "generic consts".

I'm not sure whether putting a whole SMT solver here would be reasonable, that seems like a lot of extra baggage for something that only gets rarely invoked.

I think just normalizing it for this specific use case is alright. I was also thinking about e graphs, but I'm not too familiar with it, and seems their library is a bit heavy

compiler/rustc_hir_typeck/src/intrinsicck.rs

compiler/rustc_middle/src/ty/layout.rs

JulianKnodt · 2023-01-29T04:50:56Z

will update this after I get back from vacation in a week or so

edit: am back!

nagisa · 2023-02-26T23:46:09Z

r? rust-lang/compiler

apiraino · 2023-03-22T11:57:56Z

r? rust-lang/compiler

b-naber

Probably need to take a second look at this in a couple of days, but have added some thoughts now.

compiler/rustc_middle/src/ty/layout.rs

JulianKnodt · 2023-04-07T18:57:01Z

Should be good now!

b-naber · 2023-04-08T18:10:41Z

Can anybody please explain why this needs CURRENT_RUSTC_VERSION? Not too familiar with the feature system... But I find it kind of weird that all other active features have a specific version number and the only other feature that uses CURRENT_RUSTC_VERSION is an incomplete feature.

b-naber · 2023-04-08T19:19:55Z

nvm, just saw @klensy's post.

@bors r+ rollup

bors · 2023-04-08T19:19:58Z

📌 Commit b76dd8c has been approved by b-naber

It is now in the queue for this repository.

bors · 2023-04-08T19:47:25Z

⌛ Testing commit b76dd8c with merge af06dce...

bors · 2023-04-08T21:59:43Z

☀️ Test successful - checks-actions
Approved by: b-naber
Pushing af06dce to master...

rust-timer · 2023-04-08T23:16:40Z

Finished benchmarking commit (af06dce): 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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.3%, -2.9%]	2
All ❌✅ (primary)	-	-	0

scottmcm · 2023-04-10T18:22:19Z

@b-naber The meta-reason is that it avoids the "oops, the fork happened, need to unapprove all the PRs with the version in them so they get updated with the new number" problem. A new or stabilized gate always just uses the magic constant, and the release branching scripts make sure they get the real number.

This takes a whole 3 lines in `compiler/` since it lowers to `CastKind::Transmute` in MIR *exactly* the same as the existing `intrinsics::transmute` does, it just doesn't have the fancy checking in `hir_typeck`. Added to enable experimenting with the request in <rust-lang#106281 (comment)> and because the portable-simd folks might be interested for dependently-sized array-vector conversions. It also simplifies a couple places in `core`.

…i-obk Add `intrinsics::transmute_unchecked` This takes a whole 3 lines in `compiler/` since it lowers to `CastKind::Transmute` in MIR *exactly* the same as the existing `intrinsics::transmute` does, it just doesn't have the fancy checking in `hir_typeck`. Added to enable experimenting with the request in <rust-lang#106281 (comment)> and because the portable-simd folks might be interested for dependently-sized array-vector conversions. It also simplifies a couple places in `core`. See also rust-lang#108442 (comment), where `CastKind::Transmute` was added having exactly these semantics before the lang meeting (which I wasn't in) independently expressed interest.

This takes a whole 3 lines in `compiler/` since it lowers to `CastKind::Transmute` in MIR *exactly* the same as the existing `intrinsics::transmute` does, it just doesn't have the fancy checking in `hir_typeck`. Added to enable experimenting with the request in <rust-lang/rust#106281 (comment)> and because the portable-simd folks might be interested for dependently-sized array-vector conversions. It also simplifies a couple places in `core`.

Add `intrinsics::transmute_unchecked` This takes a whole 3 lines in `compiler/` since it lowers to `CastKind::Transmute` in MIR *exactly* the same as the existing `intrinsics::transmute` does, it just doesn't have the fancy checking in `hir_typeck`. Added to enable experimenting with the request in <rust-lang/rust#106281 (comment)> and because the portable-simd folks might be interested for dependently-sized array-vector conversions. It also simplifies a couple places in `core`. See also rust-lang/rust#108442 (comment), where `CastKind::Transmute` was added having exactly these semantics before the lang meeting (which I wasn't in) independently expressed interest.

Stop using `<DefId as Ord>` in various diagnostic situations work towards rust-lang#90317 Reverts part of rust-lang#106281, as it sorts constants and that's problematic since it can contain `ParamConst`, which contains `DefId`s

Rollup merge of rust-lang#122820 - oli-obk:no_ord_def_id, r=estebank Stop using `<DefId as Ord>` in various diagnostic situations work towards rust-lang#90317 Reverts part of rust-lang#106281, as it sorts constants and that's problematic since it can contain `ParamConst`, which contains `DefId`s

rustbot assigned nagisa Dec 30, 2022

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 Dec 30, 2022

fbstj reviewed Dec 30, 2022

View reviewed changes

src/test/ui/const-generics/transmute.rs Outdated Show resolved Hide resolved

fbstj reviewed Dec 30, 2022

View reviewed changes

src/test/ui/const-generics/transmute.rs Outdated Show resolved Hide resolved

fbstj reviewed Dec 30, 2022

View reviewed changes

src/test/ui/const-generics/transmute.rs Outdated Show resolved Hide resolved

JulianKnodt force-pushed the transmute_const_generics branch 2 times, most recently from 98c8e16 to 72f169a Compare December 30, 2022 20:07

This comment has been minimized.

Sign in to view

JulianKnodt force-pushed the transmute_const_generics branch 2 times, most recently from 80f2358 to 42ca47c Compare December 30, 2022 21:06

This comment has been minimized.

Sign in to view

JulianKnodt force-pushed the transmute_const_generics branch from 42ca47c to 5dc4705 Compare December 31, 2022 00:58

nagisa added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 28, 2023

eddyb suggested changes Jan 28, 2023

View reviewed changes

JulianKnodt force-pushed the transmute_const_generics branch 2 times, most recently from 264454d to 15ef66d Compare February 3, 2023 00:17

JulianKnodt marked this pull request as ready for review February 3, 2023 00:21

JulianKnodt force-pushed the transmute_const_generics branch from 15ef66d to 001f46d Compare February 3, 2023 00:43

rustbot assigned jackh726 and unassigned nagisa Feb 26, 2023

rustbot assigned b-naber and unassigned jackh726 Mar 22, 2023

b-naber requested changes Mar 26, 2023

View reviewed changes

JulianKnodt force-pushed the transmute_const_generics branch from 001f46d to 57ec610 Compare March 30, 2023 08:20

JulianKnodt requested a review from klensy April 7, 2023 17:33

JulianKnodt force-pushed the transmute_const_generics branch from 5d9d218 to 9a0e99e Compare April 7, 2023 18:11

Add feature gate

b76dd8c

JulianKnodt force-pushed the transmute_const_generics branch from 9a0e99e to b76dd8c Compare April 7, 2023 18:18

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 8, 2023

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

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

rustbot added this to the 1.70.0 milestone Apr 8, 2023

JulianKnodt deleted the transmute_const_generics branch April 9, 2023 02:02

scottmcm mentioned this pull request Apr 10, 2023

assume value ranges in transmute #109993

Merged

scottmcm mentioned this pull request Apr 23, 2023

Add intrinsics::transmute_unchecked #110706

Merged

Mark-Simulacrum removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 30, 2023

matthiaskrgr mentioned this pull request Jun 10, 2023

ice: transmute_generic_consts: Size::bits ... bytes in bits doesn't fit in u64 #112505

Closed

oli-obk mentioned this pull request Mar 21, 2024

Stop using <DefId as Ord> in various diagnostic situations #122820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to transmute (somewhat) with generic consts in arrays #106281

Add ability to transmute (somewhat) with generic consts in arrays #106281

JulianKnodt commented Dec 30, 2022 •

edited

Loading

rustbot commented Dec 30, 2022

This comment has been minimized.

This comment has been minimized.

nagisa commented Jan 28, 2023

eddyb left a comment

eddyb Jan 28, 2023

JulianKnodt Feb 2, 2023 •

edited

Loading

JulianKnodt commented Jan 29, 2023 •

edited

Loading

nagisa commented Feb 26, 2023

apiraino commented Mar 22, 2023

b-naber left a comment

JulianKnodt commented Apr 7, 2023

b-naber commented Apr 8, 2023

b-naber commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

rust-timer commented Apr 8, 2023

scottmcm commented Apr 10, 2023

Add ability to transmute (somewhat) with generic consts in arrays #106281

Add ability to transmute (somewhat) with generic consts in arrays #106281

Conversation

JulianKnodt commented Dec 30, 2022 • edited Loading

rustbot commented Dec 30, 2022

This comment has been minimized.

This comment has been minimized.

nagisa commented Jan 28, 2023

eddyb left a comment

Choose a reason for hiding this comment

eddyb Jan 28, 2023

Choose a reason for hiding this comment

JulianKnodt Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

JulianKnodt commented Jan 29, 2023 • edited Loading

nagisa commented Feb 26, 2023

apiraino commented Mar 22, 2023

b-naber left a comment

Choose a reason for hiding this comment

JulianKnodt commented Apr 7, 2023

b-naber commented Apr 8, 2023

b-naber commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

rust-timer commented Apr 8, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

scottmcm commented Apr 10, 2023

JulianKnodt commented Dec 30, 2022 •

edited

Loading

JulianKnodt Feb 2, 2023 •

edited

Loading

JulianKnodt commented Jan 29, 2023 •

edited

Loading