Introduce NullOp::AlignOf #88839

nbdd0121 · 2021-09-10T23:58:58Z

This PR introduces Rvalue::NullaryOp(NullOp::AlignOf, ty), which will be lowered from align_of, similar to size_of lowering to Rvalue::NullaryOp(NullOp::SizeOf, ty).

The changes are originally part of #88700 but since it's not dependent on other changes and could have performance impact on its own, it's separated into its own PR.

rust-highfive · 2021-09-10T23:59:01Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2021-09-10T23:59:02Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

oli-obk · 2021-09-11T11:46:27Z

@bors try @rust-timer queue

rust-timer · 2021-09-11T11:46:29Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-09-11T11:46:35Z

⌛ Trying commit 52124553f7fb8a336b36f0de95aab8ea74f9d18b with merge 80cc2c0293bdf08aa9c0fd853e7507ce1a0272ba...

compiler/rustc_codegen_cranelift/src/base.rs

bors · 2021-09-11T13:05:28Z

☀️ Try build successful - checks-actions
Build commit: 80cc2c0293bdf08aa9c0fd853e7507ce1a0272ba (80cc2c0293bdf08aa9c0fd853e7507ce1a0272ba)

rust-timer · 2021-09-11T13:05:29Z

Queued 80cc2c0293bdf08aa9c0fd853e7507ce1a0272ba with parent 4e880f8, future comparison URL.

rust-timer · 2021-09-11T15:22:11Z

Finished benchmarking commit (80cc2c0293bdf08aa9c0fd853e7507ce1a0272ba): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

bjorn3 · 2021-09-12T08:09:45Z

compiler/rustc_middle/src/mir/mod.rs

@@ -2278,6 +2278,8 @@ impl BinOp {
 pub enum NullOp {
    /// Returns the size of a value of that type
    SizeOf,
+    /// Returns the minimum alignment of a type
+    AlignOf,


Maybe call this MinAlignOf to distinguish it from the preferred alignment?

Given that preferred alignment is never used in the library and hardly used in the compiler (in fact the only usage of preferred alignment is in cg_clif), and the name we expose min_align_of to the user is align_of, I think the name align_of/AlignOf is not ambiguous. I doubt if we'll ever add a PrefAlignOf nullary operator.

Huh, indeed. Should we remove preferred alignment then if it isn't used anyway?

Should we remove preferred alignment then if it isn't used anyway?

Yeah it might be time to remove that intrinsic... @eddyb or do you think that will ever be useful again in the future?

nagisa

Broadly LGTM, r=me with exhaustive matches back.

nagisa · 2021-09-12T20:14:55Z

compiler/rustc_const_eval/src/interpret/step.rs

@@ -270,18 +270,23 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                M::box_alloc(self, &dest)?;
            }

-            NullaryOp(mir::NullOp::SizeOf, ty) => {
+            NullaryOp(null_op, ty) => {


Please match mir::NullOp::SizeOf | mir::NullOp::AlignOf here. so that the match remains properly exhaustive and will catch additional NullOp additions in the future.

How about change the _ arm below matching null_op to a NullOp::Box arm instead? This way we still get exhaustive match and is IMO cleaner.

I don't have a strong opinion either way.

nagisa · 2021-09-12T20:20:21Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

@@ -525,6 +511,27 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                let operand = OperandRef { val: OperandValue::Immediate(val), layout: box_layout };
                (bx, operand)
            }
+
+            mir::Rvalue::NullaryOp(null_op, ty) => {


Suggested change

mir::Rvalue::NullaryOp(null_op, ty) => {

mir::Rvalue::NullaryOp(null_op@mir::NullOp::SizeOf | null_op@mir::NullOp::AlignOf, ty) => {

nagisa · 2021-09-12T20:21:31Z

compiler/rustc_codegen_cranelift/src/base.rs

@@ -726,15 +726,20 @@ fn codegen_stmt<'tcx>(
                    let ptr = fx.bcx.inst_results(call)[0];
                    lval.write_cvalue(fx, CValue::by_val(ptr, box_layout));
                }
-                Rvalue::NullaryOp(NullOp::SizeOf, ty) => {
+                Rvalue::NullaryOp(null_op, ty) => {


Suggested change

Rvalue::NullaryOp(null_op, ty) => {

Rvalue::NullaryOp(null_op@NullOp::SizeOf | null_op@NullOp::AlignOf, ty) => {

nagisa · 2021-09-12T23:10:03Z

@bors r+

bors · 2021-09-12T23:10:04Z

📌 Commit cdec87c has been approved by nagisa

bors · 2021-09-12T23:49:27Z

⌛ Testing commit cdec87c with merge 96dee28...

bors · 2021-09-13T02:45:12Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 96dee28 to master...

rust-timer · 2021-09-13T04:24:25Z

Finished benchmarking commit (96dee28): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Remove implementation of `min_align_of` intrinsic Since rust-lang#88839 `min_align_of` is lowered to AlignOf operator.

Introduce NullOp::AlignOf This PR introduces `Rvalue::NullaryOp(NullOp::AlignOf, ty)`, which will be lowered from `align_of`, similar to `size_of` lowering to `Rvalue::NullaryOp(NullOp::SizeOf, ty)`. The changes are originally part of rust-lang#88700 but since it's not dependent on other changes and could have performance impact on its own, it's separated into its own PR.

rust-highfive assigned nagisa Sep 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2021

nbdd0121 force-pushed the alignof branch from 8472654 to 5212455 Compare September 11, 2021 00:01

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 11, 2021

oli-obk reviewed Sep 11, 2021

View reviewed changes

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

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 11, 2021

nbdd0121 force-pushed the alignof branch 2 times, most recently from 6ef80f9 to 14e0cef Compare September 11, 2021 21:15

bjorn3 reviewed Sep 12, 2021

View reviewed changes

nagisa approved these changes Sep 12, 2021

View reviewed changes

nbdd0121 added 2 commits September 13, 2021 00:08

Introduce NullOp::AlignOf

1c3409f

Add mir opt test for min_align_of -> AlignOf lowering

cdec87c

nbdd0121 force-pushed the alignof branch from 14e0cef to cdec87c Compare September 12, 2021 23:08

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 Sep 12, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 13, 2021

bors merged commit 96dee28 into rust-lang:master Sep 13, 2021

rustbot added this to the 1.57.0 milestone Sep 13, 2021

nbdd0121 deleted the alignof branch September 13, 2021 04:18

oli-obk mentioned this pull request Sep 14, 2021

Remove implementation of min_align_of intrinsic #88933

Merged

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 14, 2021

Rollup merge of rust-lang#88933 - tmiasko:remove-min-align-of, r=oli-obk

c92ae5f

Remove implementation of `min_align_of` intrinsic Since rust-lang#88839 `min_align_of` is lowered to AlignOf operator.

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 14, 2021

Rollup merge of rust-lang#88933 - tmiasko:remove-min-align-of, r=oli-obk

d675005

Remove implementation of `min_align_of` intrinsic Since rust-lang#88839 `min_align_of` is lowered to AlignOf operator.

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 14, 2021

Rollup merge of rust-lang#88933 - tmiasko:remove-min-align-of, r=oli-obk

2472bbd

Remove implementation of `min_align_of` intrinsic Since rust-lang#88839 `min_align_of` is lowered to AlignOf operator.

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 14, 2021

Rollup merge of rust-lang#88933 - tmiasko:remove-min-align-of, r=oli-obk

5050a2f

Remove implementation of `min_align_of` intrinsic Since rust-lang#88839 `min_align_of` is lowered to AlignOf operator.

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 15, 2021

Rollup merge of rust-lang#88933 - tmiasko:remove-min-align-of, r=oli-obk

58cda93

Remove implementation of `min_align_of` intrinsic Since rust-lang#88839 `min_align_of` is lowered to AlignOf operator.

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 15, 2021

Rollup merge of rust-lang#88933 - tmiasko:remove-min-align-of, r=oli-obk

d11ee80

Remove implementation of `min_align_of` intrinsic Since rust-lang#88839 `min_align_of` is lowered to AlignOf operator.

RalfJung mentioned this pull request Nov 13, 2021

remove pref_align_of intrinsic #90877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce NullOp::AlignOf #88839

Introduce NullOp::AlignOf #88839

nbdd0121 commented Sep 10, 2021

rust-highfive commented Sep 10, 2021

rust-highfive commented Sep 10, 2021

oli-obk commented Sep 11, 2021

rust-timer commented Sep 11, 2021

bors commented Sep 11, 2021

bors commented Sep 11, 2021

rust-timer commented Sep 11, 2021

rust-timer commented Sep 11, 2021

bjorn3 Sep 12, 2021

nbdd0121 Sep 12, 2021

bjorn3 Sep 12, 2021

RalfJung Sep 14, 2021

nagisa left a comment

nagisa Sep 12, 2021

nbdd0121 Sep 12, 2021

nagisa Sep 12, 2021

nagisa Sep 12, 2021

nagisa Sep 12, 2021

nagisa commented Sep 12, 2021

bors commented Sep 12, 2021

bors commented Sep 12, 2021

bors commented Sep 13, 2021

rust-timer commented Sep 13, 2021

	mir::Rvalue::NullaryOp(null_op, ty) => {
	mir::Rvalue::NullaryOp(null_op@mir::NullOp::SizeOf \| null_op@mir::NullOp::AlignOf, ty) => {

	Rvalue::NullaryOp(null_op, ty) => {
	Rvalue::NullaryOp(null_op@NullOp::SizeOf \| null_op@NullOp::AlignOf, ty) => {

Introduce NullOp::AlignOf #88839

Introduce NullOp::AlignOf #88839

Conversation

nbdd0121 commented Sep 10, 2021

rust-highfive commented Sep 10, 2021

rust-highfive commented Sep 10, 2021

oli-obk commented Sep 11, 2021

rust-timer commented Sep 11, 2021

bors commented Sep 11, 2021

bors commented Sep 11, 2021

rust-timer commented Sep 11, 2021

rust-timer commented Sep 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Sep 12, 2021

bors commented Sep 12, 2021

bors commented Sep 12, 2021

bors commented Sep 13, 2021

rust-timer commented Sep 13, 2021