Make codegen choose whether to emit overflow checks #107921

cjgillot · 2023-02-11T09:04:13Z

ConstProp and DataflowConstProp currently have a specific code path not to propagate constants when they overflow. This is meant to have the correct behaviour when inlining from a crate with overflow checks (like core) into a crate compiled without.

This PR shifts the behaviour change to the Assert(Overflow*) MIR terminators: if the crate is compiled without overflow checks, just skip emitting the assertions. This is already what happens with OverflowNeg.

This allows ConstProp and DataflowConstProp to transform CheckedBinaryOp(Add, u8::MAX, 1) into const (0, true), and let codegen ignore the true.

The interpreter is modified to conform to this behaviour.

Fixes #35310

rustbot · 2023-02-11T09:04:21Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler/rustc_codegen_ssa/src/mir/block.rs

rustbot · 2023-02-11T14:56:15Z

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

RalfJung · 2023-02-11T16:06:37Z

compiler/rustc_middle/src/mir/syntax.rs

+    /// When overflow checking is disabled and we are generating run-time code, the `Overflow*`
+    /// variants of this terminator are codegened as simple `goto target`.


This is very suspicious. The spec of a language should never mention "codegen". Shouldn't this just say "when overflow checking is disabled, the Overflow* variants behave as goto target"? And then the interpreter should also implement those semantics.

Ah, this is partially copied from the existing CheckedBinaryOp docs, but the partial copy made it even more confusing. That existing doc should also be adjusted...

Suggested change

/// When overflow checking is disabled and we are generating run-time code, the `Overflow*`

/// variants of this terminator are codegened as simple `goto target`.

/// When overflow checking is disabled and this is run-time MIR (as opposed to

/// compile-time MIR that is used for CTFE), the `Overflow*`

/// variants of this terminator behave as `goto target`.

Having semantic differences between runtime and compiletime MIR is not great but is not new either.

And the interpreter will still need adjustments since Miri executes runtime MIR.

compiler/rustc_codegen_ssa/src/mir/block.rs

RalfJung · 2023-02-11T16:18:57Z

The specific code path to codegen CheckedBinaryOp in the absence of overflow checks is kept as an optimization.

I don't understand what you mean by this. Without overflow checks MR building should generate non-checked BinaryOp, no?

rustbot · 2023-02-11T16:45:10Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

cjgillot · 2023-02-11T16:52:55Z

The specific code path to codegen CheckedBinaryOp in the absence of overflow checks is kept as an optimization.

I don't understand what you mean by this. Without overflow checks MR building should generate non-checked BinaryOp, no?

Libraries may be built with overflow checks enabled, so emit MIR with checked operations, and then that MIR may be codegened into a non-checked compilation (through inlining or monomorphization). In that case, the downstream crate dictates the behaviour and must remove the overflow checks.

As for the optimization, it refers to this code.

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

Lines 654 to 661 in 5b45024

    
           // This case can currently arise only from functions marked 
        
           // with #[rustc_inherit_overflow_checks] and inlined from 
        
           // another crate (mostly core::num generic/#[inline] fns), 
        
           // while the current crate doesn't use overflow checks. 
        
           if !bx.cx().check_overflow() { 
        
               let val = self.codegen_scalar_binop(bx, op, lhs, rhs, input_ty); 
        
               return OperandValue::Pair(val, bx.cx().const_bool(false)); 
        
           }

RalfJung · 2023-02-11T16:57:39Z

Libraries may be built with overflow checks enabled, so emit MIR with checked operations, and then that MIR may be codegened into a non-checked compilation (through inlining or monomorphization).

I don't think that is accurate in general -- libraries are built with the flags determined by the main crate cargo builds.

There is one special exception, and it is the #[rustc_inherit_overflow_checks] attribute used in the standard library. AFAIK that is the only reason these special cases in const-prop are needed.

As for the optimization, it refers to this code.

Ah, that's what I missed, thanks. Would be good to add a comment saying that this is just an optimization now since the assertion anyway becomes a NOP.

compiler/rustc_middle/src/mir/syntax.rs

compiler/rustc_const_eval/src/interpret/terminator.rs

compiler/rustc_codegen_ssa/src/mir/block.rs

compiler/rustc_middle/src/mir/syntax.rs

cjgillot · 2023-02-11T21:51:05Z

@bors try @rust-timer queue

bors · 2023-02-11T21:51:15Z

⌛ Trying commit c9cd5659b54a45ce4a6cddf03de5a11cd8de15bb with merge fca0d6e1a6baaaf9af26313a4bd254bc19f51f39...

bors · 2023-02-12T00:08:27Z

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

rust-timer · 2023-02-12T04:32:58Z

Finished benchmarking commit (fca0d6e1a6baaaf9af26313a4bd254bc19f51f39): 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

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

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)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

compiler/rustc_mir_transform/src/lower_intrinsics.rs

tmiasko · 2023-02-19T08:20:35Z

@bors r+

bors · 2023-02-19T08:20:37Z

📌 Commit 9f6c1df has been approved by tmiasko

It is now in the queue for this repository.

bors · 2023-02-19T11:07:29Z

⌛ Testing commit 9f6c1df with merge 2e62ed984d82cb78475414ccb28aac6b0a08bee0...

bors · 2023-02-19T11:17:24Z

💔 Test failed - checks-actions

tmiasko · 2023-02-19T11:19:43Z

@bors retry #108227

rust-log-analyzer · 2023-02-19T14:32:04Z

The job x86_64-mingw-2 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[2615/3025] Linking CXX static library lib\libLLVMAsmPrinter.a
[2616/3025] Linking CXX static library lib\libLLVMMIRParser.a
[2617/3025] Linking CXX static library lib\libLLVMInterpreter.a
FAILED: lib/libLLVMInterpreter.a 
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E rm -f lib\libLLVMInterpreter.a && D:\a\rust\rust\mingw64\bin\ar.exe qc lib\libLLVMInterpreter.a  lib/ExecutionEngine/Interpreter/CMakeFiles/LLVMInterpreter.dir/Execution.cpp.obj lib/ExecutionEngine/Interpreter/CMakeFiles/LLVMInterpreter.dir/ExternalFunctions.cpp.obj lib/ExecutionEngine/Interpreter/CMakeFiles/LLVMInterpreter.dir/Interpreter.cpp.obj && D:\a\rust\rust\mingw64\bin\ranlib.exe lib\libLLVMInterpreter.a && cd ."
D:\a\rust\rust\mingw64\bin\ar.exe: could not create temporary file whilst writing archive: no more archived files
[2618/3025] Building CXX object tools/llvm-as/CMakeFiles/llvm-as.dir/llvm-as.cpp.obj
[2619/3025] Linking CXX static library lib\libLLVMWebAssemblyUtils.a
[2620/3025] Linking CXX static library lib\libLLVMDWARFLinker.a
[2621/3025] Building CXX object tools/lli/CMakeFiles/lli.dir/lli.cpp.obj
[2621/3025] Building CXX object tools/lli/CMakeFiles/lli.dir/lli.cpp.obj
[2622/3025] Linking CXX static library lib\libLLVMSelectionDAG.a
[2623/3025] Linking CXX executable bin\llvm-profdata.exe
[2624/3025] Linking CXX static library lib\libLLVMPasses.a
ninja: build stopped: subcommand failed.
command did not execute successfully, got: exit code: 1


build script failed, must exit now', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cmake-0.1.48\src\lib.rs:975:5
 finished in 219.037 seconds
Build completed unsuccessfully in 0:04:54
Build completed unsuccessfully in 0:04:54
make: *** [Makefile:80: ci-mingw-subset-2] Error 1

bors · 2023-02-19T18:17:29Z

⌛ Testing commit 9f6c1df with merge 7aa413d...

bors · 2023-02-19T21:18:01Z

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing 7aa413d to master...

rust-timer · 2023-02-19T22:23:43Z

Finished benchmarking commit (7aa413d): 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)

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

Cycles

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

rustbot assigned oli-obk Feb 11, 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 Feb 11, 2023

cjgillot changed the title ~~Make codegen's choose whether to emit overflow checks~~ Make codegen choose whether to emit overflow checks Feb 11, 2023

cjgillot force-pushed the codegen-overflow-check branch from 2f6b9ea to cd152c4 Compare February 11, 2023 09:32

tmiasko reviewed Feb 11, 2023

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved

cjgillot mentioned this pull request Feb 11, 2023

Lift a few limitations on ConstProp optimization pass #107856

Closed