Improve GPU codegen by defaulting to immediate-abort

[ZuseZ4]

When looking at the IR of some GPU benchmarks, we noticed that even with panic=abort we ended up with quite a lot of extra IR from the panic machinery. With panic=immediate-abort, those become simple branches to a trap call:

     1 panic:                                            ; preds = %bb2
  217    tail call void @llvm.trap()
     1   unreachable
     2 

We mostly see it being generated from bounds checks.

Specifying immediate-abort by hand is quite annoying.

1. you need to enable the experimental cargo feature for panic-abort. That for example means your stable cargo won't understand your project anymore and refuse even simple commands like cargo clean.
2. You need an extra (semi-hard to find) feature for build std -Zbuild-std=core,panic_abort
3. (might be offload specific) we don't necessarily want immediate-abort on the host, so specifying immediate-abort as panic strategy in your Cargo.toml has undesired side-effects.

The only benefit of abort over immediate-abort is the ability to hook in your own panic handler. I think that's a rarely needed feature on GPUs, so we should prioritize better codegen, especially if it's so annoying to specify by hand otherwise.

@Flakebi @kjetilkjeka @kulst are you ok with this as target maintainers?

r? @saethlin

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[rust-log-analyzer]

The job aarch64-gnu-llvm-21-1 failed! Check out the build log: (web) (plain enhanced) (plain)

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

[TIMING:end] test::TestHelpers { target: aarch64-unknown-linux-gnu } -- 0.072
##[group]Testing stage2 with compiletest suite=ui mode=ui (aarch64-unknown-linux-gnu)

thread 'main' (23159) panicked at src/tools/compiletest/src/common.rs:928:10:
called `Result::unwrap()` on an `Err` value: Error("unknown variant `immediate-abort`, expected `unwind` or `abort`", line: 2276, column: 39)
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/panicking.rs:689:5
   1: core::panicking::panic_fmt
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/core/src/panicking.rs:80:14
   2: core::result::unwrap_failed
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/core/src/result.rs:1867:5
   3: <compiletest::common::TargetCfgs>::new
   4: <std::sync::once::Once>::call_once_force::<<std::sync::once_lock::OnceLock<compiletest::common::TargetCfgs>>::initialize<<std::sync::once_lock::OnceLock<compiletest::common::TargetCfgs>>::get_or_init<<compiletest::common::Config>::target_cfgs::{closure#0}>::{closure#0}, !>::{closure#0}>::{closure#0}
   5: <std::sys::sync::once::futex::Once>::call
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/sys/sync/once/futex.rs:183:21
   6: <std::sync::once_lock::OnceLock<compiletest::common::TargetCfgs>>::initialize::<<std::sync::once_lock::OnceLock<compiletest::common::TargetCfgs>>::get_or_init<<compiletest::common::Config>::target_cfgs::{closure#0}>::{closure#0}, !>
   7: compiletest::directives::cfg::prepare_conditions
   8: <compiletest::directives::DirectivesCache>::load
   9: compiletest::run_tests
  10: compiletest::cli::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Bootstrap failed while executing `--stage 2 test --skip tidy --skip compiler --skip src`
Build completed unsuccessfully in 0:25:43

[Flakebi]

The only benefit of abort over immediate-abort is the ability to hook in your own panic handler. I think that's a rarely needed feature on GPUs

Hm, I see the point of not wanting to pay for panicking. On the other hand, I did use panic handlers. It’s nice to get an error message if an assert!() fails (or when accessing a slice out-of-bounds, etc.).

[ZuseZ4]

Hm, would you be open to setting panic=abort while debugging? My concern is that most people will just run everything in the default config, in which they would (currently) pay the price for panic hooks without using them. I don't really believe that you can get the average person to read our compiler docs when everything seems to run fine, so I care about the default performance more.
When things crash, I feel like they're much more likely to read up on debuggers and flags, so them learning to set panic=abort and including your panic handler is hopefully straightforward, especially since =abort is much easier to get working than =immediate-abort?

[Flakebi]

Could we have abort for debug mode and immediate-abort for release mode?
(Then one could compile in debug with optimizations)

Edit: So, with debug-assertions you would get abort and without it would be immediate-abort.
I think it would be a bad experience if you run in debug mode and don’t get error messages, but it just hangs.
Similarly, it would be a bad experience if release mode is slower than it can be by default.

[bjorn3]

Not easily. If any crate is compiled with panic=immediate-abort, then all crates must be compiled with it, including the standard library. In fact panic=immediate-abort mostly works by swapping out some functions in libcore.

[kulst]

Hm, I see the point of not wanting to pay for panicking. On the other hand, I did use panic handlers. It’s nice to get an error message if an assert!() fails (or when accessing a slice out-of-bounds, etc.).

^ Exactly this. It's unfortunate, that specifying it by hand is currently not so easy. Having abort in Debug mode and immediate-abort in Release mode would indeed be great.

Not easily. If any crate is compiled with panic=immediate-abort, then all crates must be compiled with it, including the standard library. In fact panic=immediate-abort mostly works by swapping out some functions in libcore.

I wouldn't consider the necessity to recompile everything a problem for our use case. For amdgcn and NVPTX we have to do this anyway to produce correct code. To me it looks like this is mostly a build system problem since the Cargo flags required to do that are experimental and therefore are not nicely integrated into it.

[kjetilkjeka]

I guess the bloat related to the bounds checks doesn't come directly from the bounds check themselves as they still need to run? Then I assume the difference in the IR level could be a trap or a call to the panic handler, but inlining of the panic handler happens and whatever goes on there is copied into the function itself?

If the panic handler is defined as a trap instruction, will the two versions end up the same in the final artifact (ptx for nvptx) such that this is mostly a bloated LLVM-IR/BC issue?

Using a custom panic handler with stdout printing together with a "release with debug" profile to immediately expose invariants that are not upheld is a valuable use case. Using explicit panic=abort is fine, but I'm just noting that if this doesn't give the users "better code" it's probably a slight negative from a UX perspective

[saethlin]

If the panic handler is defined as a trap instruction, will the two versions end up the same in the final artifact (ptx for nvptx)

You can already define the panic handler as a trap instruction in user code. If changing the panic handler suffices, you don't even need to have this conversation.

I strongly suspect that simply changing the panic handler will not clean up enough code, because the panic handler is called over FFI (think about it, that's how the panic handler implementation which is called into by core is swapped out from a crate that depends on core) and because the actual calls to the panic handler are behind (in some cases multiple) #[cold] #[inline(never)] function calls, which are normally bad inlining candidates but become good ones only when the panic handler is known to be just a trap instruction.

The only way you'd be able to get the code cleaned up just by swapping the panic handler is if something runs after LLVM and supports LTO-style optimizations. Normally I'd assume that no such optimizations exist, but you say

such that this is mostly a bloated LLVM-IR/BC issue?

Which only makes sense if they do 🤷 so you tell me?

[kjetilkjeka]

For NVPTX, we normally use build-std together with fat LTO. This means optimizations run on a single unit containing the “business logic,” core, and the panic handler. I took a practical approach to determine whether this was possible without immediate-abort, and as predicted, #[inline(never)] appears to prevent it. If I manually strip the noinline attributes, LLVM inlines the calls. This confirms that immediate-abort provides real value to users through smaller binaries. Of course, not having to provide a panic handler for a minimal example is also nice.

Is this something that could be coordinated across no_std targets? My main concern is introducing target-specific behavior for the debug profile instead of behaving as Rust normally does.

I also do not see any other targets using immediate-abort. I do not know the future plans for this feature, but using an unstable feature as the default when no other targets do so seems concerning.

This would also change the behavior of existing working code by silently ignoring its panic handler. At the very least, that would require properly notifying users.

Given my limited knowledge of the future plans for this feature and for no_std targets, my preferred solution would be to use immediate-abort for the benchmarks and add a section to the NVPTX target documentation about minimizing binary size that mentions this feature.

It is also unfortunate that enabling it for the offload device target enables it for the host as well. Is support for separate configurations for the host and offload device planned anyway?