Repository navigation
[ROCm] Use fmaxf in maxdistanceCuboid so voxel sizes are not truncated - #2
Merged
Merged
Conversation
maxdistanceCuboid() is host code in a .cu file and calls unqualified max() on floats. CUDA declares host min/max overloads for float and double, but HIP's host math header only declares max(int, int), so under hipcc the float arguments are silently converted to int. In the 3D-angle branch (theta or psi nonzero) the largest voxel size becomes an integer: for voxel sizes above 1 the interpolated projector (cone and parallel) starts sampling too far along the ray and skips part or all of the object, and for voxel sizes below 1 it divides by zero. With dVoxel 1.5, theta 0.3 and psi 0.2 the interpolated projection of a 64^3 phantom is all zeros where Siddon gives a sum of 2.1e5. Call fmaxf, which is what CUDA's max(float, float) resolves to, at the four float max() calls in the function. CUDA results, NaN handling included, are unchanged. The 2D-angle branch gets the same change; its result is floored by the caller, so its output does not change. Device code is unchanged. Add a test comparing the interpolated and Siddon projectors with voxel sizes 0.8 and 1.5 and nonzero theta and psi, for cone and parallel beam. Before this change the two 1.5 cases fail on ROCm with a relative error of 1.0. Authored with Claude. Test Plan: ``` BUILD_WITH_HIP=1 HIP_ARCH=gfx90a python setup.py build_ext --inplace cd Python python -m pytest tests/test_interpolated_voxel_size.py tests/test_compute_cor.py tests/test_demo_angles.py tests/test_denoise_2d_guard.py tests/test_filtering_workers.py tests/test_gpuids_default.py tests/test_krylov_restart_and_norms.py tests/test_krylov_smoke.py ```
Collaborator
Author
|
To approve this fix round, leave a comment containing this line by itself: To send it back to the porter instead: Approving covers the commits on this branch and the section under '## Upstream reply' in the body, which is posted verbatim as a comment on the upstream pull request after the merge, behind a standing line disclosing that it was drafted by an AI assistant. |
Collaborator
Author
|
/moat approve |
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.
A host-side max() call in the interpolated forward projector truncated floats to int under hipcc. This round adds one commit (f779703) on top of the published tip b61ff14. No maintainer asked for it; we found it while checking ported projects for host min/max calls on floating-point values.
Cause: maxdistanceCuboid() in Common/CUDA/ray_interpolated_projection.cu is host code and calls max() on floats. CUDA has host min/max overloads for float and double; HIP's host header only has max(int, int), so hipcc converts the floats to int without a diagnostic. With theta or psi nonzero, the largest voxel size became an integer: above 1 the projector (cone and parallel) started sampling past part or all of the object (dVoxel 1.5 and 2.5 gave all-zero projections on MI250X), and below 1 it divided by zero.
Fix: the four float max() calls in that function now call fmaxf, which is what CUDA's max(float, float) resolves to, so CUDA results are unchanged. Device code is byte-identical. A new test, Python/tests/test_interpolated_voxel_size.py, compares the interpolated and Siddon projectors at voxel sizes 0.8 and 1.5 with nonzero theta and psi; on b61ff14 the two 1.5 cases fail with relative error 1.0, at f779703 all 4 pass.
Audit: a host-only hipcc pass over all 15 .cu files, with deleted template min/max to catch every call that falls to (int, int), found no other float or double call. The remaining hits are unsigned or mixed int counts that agree with CUDA in their ranges, except voxel_backprojection.cu:732, where with many image splits CUDA's unsigned max wraps and HIP returns the intended 0; it is left as is and listed in the round's notes.
Retested at f779703 on MI250X (gfx90a, Linux), Radeon PRO W7800 (gfx1100, Linux) and Radeon 8060S (gfx1151, Windows): 8/8 extensions build, the 7 upstream test files pass (70) and the new file passes (4); all projection, reconstruction and adjointness checks match b61ff14 exactly, and so do the repeat (race) checks on Linux (the laptop runs no repeat loops). nvcc 12.8: all 8 extensions build, with the same nvcc warnings as b61ff14; device code of the changed file is byte-identical. On a Tesla V100 (CUDA 12.9), b61ff14 and f779703 both build, the 7 upstream test files give 70 passed on both, and the new test passes 4/4 on both, as expected since CUDA never truncated.
Review the delta alone: f779703 (one commit, no merge).
Upstream reply
I added a follow-up commit that fixes the interpolated forward projector on AMD GPUs when theta or psi is nonzero and the voxel size is not a whole number: a host max() call cut the voxel size to an integer, so for sizes above 1 part or all of the object was skipped. It also adds a small test that checks the interpolated projector against Siddon for that case. On NVIDIA nothing changes (the call now names fmaxf, which CUDA's max(float, float) already called); the new test and the existing Python tests pass on a V100 before and after.
Notes for the approver
The upstream reply above is the only part posted on CERN#747.