Skip to content

Lower f32 max/min to native max.f32/min.f32 - #1668

Open
CanReader wants to merge 1 commit into
NVIDIA:mainfrom
CanReader:perf/f32-minmax-native
Open

CanReader wants to merge 1 commit into
NVIDIA:mainfrom
CanReader:perf/f32-minmax-native

Conversation

@CanReader

Copy link
Copy Markdown
Contributor

Summary

f32 max/min now lower to native max.f32/min.f32 instead of setp + selp. f64 and the libNVVM backend are unchanged. Refs #1594.

Changes

  • f32 max/min call llvm.nvvm.fmax.f / llvm.nvvm.fmin.f on the LLVM NVPTX backend.
  • Added lowering tests for both backends and a float_minmax example.

Testing

RTX 4050 (sm_89), CUDA 13.4:

  • float_minmax: all 256 pairs of NaN (quiet and signaling), infinity, signed zero and subnormal values match Rust's contract.
  • PTX: 8 max.f32/min.f32, no .ftz, no f32 setp/selp. SASS: 8 FMNMX.
  • Signed zero: max(-0.0, +0.0) and max(+0.0, -0.0) both give +0.0. min gives -0.0 in both orders. The old lowering returned the second operand.
  • No flush-to-zero setting exists in the pipeline, so only the non-.ftz form is emitted.
  • mir-lower and dialect-mir tests, clippy and cargo oxide fmt --check pass.
  • Not run: cargo-deny, just check-intrinsics, the full just check.

Checklist

  • All commits signed off
  • SPDX headers on new source files

Refs NVIDIA#1594.

Co-authored-by: imanolgo <858793+ImanolGo@users.noreply.github.com>
Signed-off-by: Canberk Pitirli <canberkpitirli@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 28, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@nihalpasham nihalpasham added the cuda-oxide SIMT programming model: rustc backend, cargo oxide, cuda-device, cuda-host, book label Oct 29, 2026

@ImanolGo ImanolGo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I re-profiled the kernel from #1594 on sm_75 and read through the change.

Setup. GTX 1650 Max-Q (sm_75), driver 615.78.08, CUDA 13.4, LLVM NVPTX backend. The same flock kernel (murmuration-kernels, oxide/) built against this PR's base (a4a939a) and head (a48c53e), each with its own cargo-oxide and backend cache.

Code generation for the neighbour-search kernel decide (ptxas -arch=sm_75 -O3, cuobjdump):

decide base this PR
PTX max.f32 + min.f32 0 18
PTX setp.*.f32 / selp.f32 106 / 42 73 / 6
SASS instructions 1,440 1,376
SASS FMNMX / FSETP / FSEL 0 / 116 / 28 18 / 83 / 4
registers 64 64

For reference, the CUDA C++ version of this kernel has 20 FMNMX, so this closes nearly all of that gap.

Correctness. Both builds pass the repo's parity check against its CPU reference (2,000 birds, pointwise at 1, 10, 30 and 100 steps, statistics at 1,000).

Performance. decide, mean GPU time over 20 steps after 20 warm-up, three runs per build interleaved, median:

birds base this PR change
100,000 8.187 ms 8.133 ms -0.7 % (each PR run faster than each base run)
1,000,000 100.92 ms 100.77 ms -0.1 %

Nsight Compute, one decide launch at 100,000 birds: instructions executed 255.8 M to 253.7 M (-0.8 %), FP32 thread instructions -2.4 %, achieved occupancy unchanged (70 %).

So the fix does what #1594 asked: the compare/select pattern is gone and the SASS uses FMNMX. In #1594 I couldn't isolate what the min/max lowering cost on its own; on this kernel it turns out to be under 1 % of time and instructions, so most of the gap to CUDA C++ there is elsewhere (bounds checks were about 9 % of instructions). I couldn't run sm_89, because I don't have an Ada GPU at hand right now.

Code. The dispatch only reaches llvm_nvvm_fmax_f / llvm_nvvm_fmin_f under IntrinsicBackend::LlvmNvptx; libNVVM goes straight to libdevice_name and keeps the expansion, and the two new lowering tests pin both cases. The example's NaN checks (quiet and signaling, both signs, both operand positions, bit-exact) cover the contract that ruled out llvm.maxnum in #390. Leaving signed zeros unconstrained matches Rust. Looks good to me (I'm not a maintainer).

AI contribution: measured and drafted with Claude Code (Anthropic); reviewed and posted by me.

@ImanolGo ImanolGo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving from my side as the reporter of #1594 (not a maintainer): the change fixes the lowering, keeps libNVVM and f64 on the NaN-safe expansion, and passes my kernel's parity check on sm_75. Measurements are in my review above.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda-oxide SIMT programming model: rustc backend, cargo oxide, cuda-device, cuda-host, book

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants