Add `#[optimize(none)]` by clubby789 · Pull Request #128657 · rust-lang/rust

clubby789 · 2024-08-04T18:08:10Z

This extends the optimize attribute to add none, which corresponds to the LLVM OptimizeNone attribute.

Not sure if an MCP is required for this, happy to file one if so.

rustbot · 2024-08-04T18:08:17Z

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

clubby789 · 2024-08-04T18:45:53Z

While writing a codegen test, looks like there's some extra work to do -

pub fn none() -> i32 {
    2 + 2
}

emits overflow checks in opt-level=0 but not =1. It looks like with overflow checks disabled this is constant folded to 4, even in opt-level=0

cjgillot · 2024-08-04T19:15:52Z

You may need to skip mir opts too.

clubby789 · 2024-08-04T19:17:04Z

I've turned off MIR opts in the tests for now, but I think ideally we'd want to skip running them for #[optimize(none)] functions - will look into that
While we could enable overflow checks (to match opt-level=0), I feel like if the main goal of opt-none is to make debugging easier then leaving them off is probably best.

clubby789 · 2024-08-04T19:51:30Z

Unhelpfully, LLVM doesn't seem to fully account for optnone: https://godbolt.org/z/8hs17bc44 - the constant is propogated from the inner function despite optnone: https://groups.google.com/g/llvm-dev/c/lqgPRW_akSU

rustbot · 2024-08-04T20:13:25Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

clubby789 · 2024-08-04T20:17:59Z

compiler/rustc_mir_transform/src/lib.rs

I don't think this is actually correct - some of these should run in debug mode. Not sure how best to decide which passes should be omitted

This is indeed incorrect. There are required passes that must be run on every MIR for soundness IIRC.

@rust-lang/wg-mir-opt: Anyone willing to provide assistance to how to disable MIR optimizations while only keeping the required passes?

Maybe check it against the opt level of the pass. If it runs on opt level 0, then don't disable it

From a brief look, there are 3 or 4 passes that need keeping, including marking the mir phase.
The easiest way would be to compute the list of passes based on the opt level, or tweak the pass manager to compare default opt level with this flag.

compiler/rustc_attr/src/builtin.rs

compiler/rustc_mir_build/src/build/mod.rs

fee1-dead · 2024-08-06T04:44:17Z

compiler/rustc_mir_transform/src/lib.rs

This is indeed incorrect. There are required passes that must be run on every MIR for soundness IIRC.

@rust-lang/wg-mir-opt: Anyone willing to provide assistance to how to disable MIR optimizations while only keeping the required passes?

compiler/rustc_middle/src/mir/mod.rs

cjgillot · 2024-08-06T10:25:02Z

compiler/rustc_mir_transform/src/lib.rs

From a brief look, there are 3 or 4 passes that need keeping, including marking the mir phase.
The easiest way would be to compute the list of passes based on the opt level, or tweak the pass manager to compare default opt level with this flag.

clubby789 · 2024-08-06T12:19:01Z

Pushed an approach which adds a min_mir_opt_level to the pass trait, then removes any which are > 1 before running opts - this does seem a bit invasive

cjgillot · 2024-08-10T11:10:54Z

@bors try @rust-timer queue

Add `#[optimize(none)]` cc rust-lang#54882 This extends the `optimize` attribute to add `none`, which corresponds to the LLVM `OptimizeNone` attribute. Not sure if an MCP is required for this, happy to file one if so.

bors · 2024-08-10T11:12:06Z

⌛ Trying commit 4408092 with merge 334c324...

bors · 2024-08-10T13:06:05Z

☀️ Try build successful - checks-actions
Build commit: 334c324 (334c3248c902c016532b42926eaa3defca560875)

rust-timer · 2024-08-10T15:20:28Z

Finished benchmarking commit (334c324): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.4%, secondary 3.6%)

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

Cycles

Results (secondary 2.0%)

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

Binary size

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

Bootstrap: 762.637s -> 762.18s (-0.06%)
Artifact size: 339.23 MiB -> 339.29 MiB (0.02%)

Co-authored-by: Waffle Lapkin <waffle.lapkin@gmail.com>

compiler/rustc_attr_data_structures/src/attributes.rs

WaffleLapkin · 2025-01-24T19:58:40Z

@bors r=fee1-dead,WaffleLapkin

bors · 2025-01-24T19:58:43Z

📌 Commit 5ac95a5 has been approved by fee1-dead,WaffleLapkin

It is now in the queue for this repository.

bors · 2025-01-25T05:50:40Z

⌛ Testing commit 5ac95a5 with merge 6365178...

bors · 2025-01-25T08:55:33Z

☀️ Test successful - checks-actions
Approved by: fee1-dead,WaffleLapkin
Pushing 6365178 to master...

rust-timer · 2025-01-25T10:22:39Z

Finished benchmarking commit (6365178): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.2%, secondary -5.2%)

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

Cycles

Results (primary 1.2%, secondary 1.7%)

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.2%	[1.0%, 1.4%]	2
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.0%, 1.4%]	2

Binary size

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

Bootstrap: 772.322s -> 771.261s (-0.14%)
Artifact size: 325.82 MiB -> 325.82 MiB (-0.00%)

scottmcm · 2025-02-03T17:35:37Z

It looks like with overflow checks disabled this is constant folded to 4, even in opt-level=0

Based on what I found in #135760 (comment), it looks like the LLVM APIs won't emit add i32 2, 2, and will always just fold that.

So it's expected that even with optimize(none) that the IR won't have that addition.

…ired, r=cjgillot compiler: Don't mark `SingleUseConsts` MIR pass as "required for soundness" I don't think this MIR pass is required for soundness. The reasons are: * Something like it was not enabled by default before PR rust-lang#107404 which was the precursor to `SingleUseConsts` (see rust-lang#125910 for the switch). * By following the advice from rust-lang#128657 (comment) we can conclude it is not required for soundness since it has only ever run on MIR opt level > 0. * Its [`MirPass::can_be_overridden()`](https://github.com/rust-lang/rust/blob/0ee7d96253f92b15115c94a530db8b79cb341b15/compiler/rustc_mir_transform/src/pass_manager.rs#L98-L102) is unchanged and thus returns `true`, indicating that it is not a required MIR pass. * PR CI pass in rust-lang#151426 which stops enabling it by default in non-optimized builds. As shown in the updated test `tests/mir-opt/optimize_none.rs`, `#[optimize(none)]` functions become even less optimized, as expected and desired. Unblocks rust-lang#151426.

Rollup merge of #152729 - Enselic:single_use_consts-not-required, r=cjgillot compiler: Don't mark `SingleUseConsts` MIR pass as "required for soundness" I don't think this MIR pass is required for soundness. The reasons are: * Something like it was not enabled by default before PR #107404 which was the precursor to `SingleUseConsts` (see #125910 for the switch). * By following the advice from #128657 (comment) we can conclude it is not required for soundness since it has only ever run on MIR opt level > 0. * Its [`MirPass::can_be_overridden()`](https://github.com/rust-lang/rust/blob/0ee7d96253f92b15115c94a530db8b79cb341b15/compiler/rustc_mir_transform/src/pass_manager.rs#L98-L102) is unchanged and thus returns `true`, indicating that it is not a required MIR pass. * PR CI pass in #151426 which stops enabling it by default in non-optimized builds. As shown in the updated test `tests/mir-opt/optimize_none.rs`, `#[optimize(none)]` functions become even less optimized, as expected and desired. Unblocks #151426.

rustbot assigned fee1-dead Aug 4, 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 Aug 4, 2024

rust-cloud-vms bot force-pushed the optimize-none branch from 42ac3f0 to 94815e5 Compare August 4, 2024 19:15