Conversation
LngelKyo
marked this pull request as draft
September 18, 2026 18:08
LngelKyo
marked this pull request as ready for review
September 18, 2026 20:52
LngelKyo
force-pushed
the
fix/cuda-min-max-nan
branch
from
September 21, 2026 22:53
c87055f to
4232dd4
Compare
LngelKyo
marked this pull request as draft
September 21, 2026 22:53
LngelKyo
force-pushed
the
fix/cuda-min-max-nan
branch
from
September 21, 2026 23:52
cbe6a06 to
5d7a3a0
Compare
The CUDA codegen prints bare min()/max() for T.min/T.max, so a NaN operand is dropped: with apache#20054's data only 4 of 8 lanes match bitwise, and the +-0 ties disagree with the C host. Emit ((a > b) || (a != a)) ? a : b for max (and the < form for min), matching apache#20054's host-side semantics: a NaN on either side survives and ties take b. Covers float16/bfloat16/float32/float64; integer and other non-float min/max keep the base CodeGenC path, including the per-lane vector expansion. Only the CUDA codegen is touched; the host side is apache#20054's. Operands are bound once via SSAGetID inside their own scope. The scope matters: SSAGetID caches by printed text within the live scope and the BufferStore dispatch does not invalidate it, so without it warp allreduce -- which emits red_buf[0] = max(red_buf[0], shuffle_down(...)) repeatedly -- reads the pre-write value from the second statement on. Constant operands take the simplified forms raised in the apache#20054 review: a constant NaN lhs is the result, a constant NaN rhs needs only the NaN test, and a constant non-NaN operand drops the NaN clause, using a reversed non-strict compare when the constant is on the right. Vector constants keep the general per-lane path. Tests go in tests/python/codegen/test_target_codegen_cuda.py, gpu-marked and covering both compile paths through the autouse fixture: apache#20054's data over four dtypes x {scalar, vec4}, the constant-operand matrix, nested uses that would expose a missing parenthesis, a chained-statement case that pins the SSA scoping, and an int32 vec4 regression against numpy.
LngelKyo
force-pushed
the
fix/cuda-min-max-nan
branch
from
September 22, 2026 00:08
b629c8e to
6810d51
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug: on
main, the CUDA codegen prints baremin()/max()forT.min/T.maxon float32/float16/bfloat16, so a NaN operand is droppedWith #20054's test data, the CUDA target gives 4/8 bitwise matches: the NaN lanes are discarded and the ±0 ties disagree with the C host.
Fix
Same semantics as #20054: emit
(((a > b) || (a != a)) ? a : b)formax(and the<form formin), so a NaN on either side is preserved and ties takeb. Scalar form binds each operand once via SSA and references the temporaries; the vector form expands the same expression per lane. Covers float16/bfloat16/float32/float64. Integer and other non-float min/max keep the existing base-CodeGenCpath, including the per-lane vector expansion. Only the CUDA codegen is touched; the host side is #20054's.Generated source (scalar and per-lane vector), from the branch:
Tests
Folded into
tests/python/codegen/test_target_codegen_cuda.py: 4 dtypes (float32/float64/float16/bfloat16) × 2 ops × {scalar, vec4}, a composite case (C[i] = max(A[i], B[i]) + 1.0— catches the ternary being parsed as(x + cond) ? va : vb; NaN lanes assert NaN, finite lanes bitwise), and an int32 vec4 regression against numpy. Both compile paths (nvcc, nvrtc) are covered by the autouse fixture.A further defect surfaced in CI: the scalar path's SSA bindings stayed alive across statements.
SSAGetIDcaches by printed text within the live scope and the base-codegenBufferStoredispatch does not invalidate it, so warp allreduce — which emitsred_buf[0] = max(red_buf[0], shuffle_down(...))repeatedly — read the pre-write value from the second statement on: the first CI run had 92 failures in test_gpu_codegen_allreduce, all finite values with max systematically too small. The scalar bindings are now scoped to the statement (BeginScope/EndScope, the same shape the vector path already had). A chained-statements test (C[v] = max(C[v], A[v]); C[v] = max(C[v], B[v]),Cpre-filled) fails on the pre-fix code and passes with the fix.Verification
RTX A6000, CUDA 13.0.88:
test_target_codegen_cuda.py: 404 passed, 6 skipped;return false): 34 failed; the 4 int cases and the 2 chained cases stay green, the latter by construction since the control bypasses the new path;ruff@0.12.3check/format andclang-format 20.1.8 --dry-run --Werrorclean.Partially addresses #19579 (CUDA side); host side is #20054.
cc @tlopex @yongwww @swjng — CI will likely need approval as before.