Response to the upstream review of PR #186 - #1
Merged
Conversation
Every warp shuffle in PopSift operates on a group of 32 threads that comes from the launch configuration (a descriptor tile, an image row, one row of the normalization block, the bitonic network), not from the hardware warp size. Passing that width explicitly states the intent, is correct when a wavefront is wider than the group, and removes the per-platform branch that used to spell the same reduction twice. The lane zero broadcast that picks the strongest orientation is one of those groups: ori_par launches a (32,1) block, so the width names the block row there as well. The extremum counter is the exception, because it counts over a whole wavefront, and its shuffles keep the hardware width. On NVIDIA the width equals the warp size, so the generated code and the results are unchanged. The compatibility header no longer redefines the __shfl_*_sync builtins. HIP provides the mask-free __shfl/__ballot/__any/__all spelling with a width parameter, which is the same spelling assist.h selects for CUDA before 9.0, so the AMD build now simply takes that branch through PopSift_HAVE_SHFL_DOWN_SYNC. That also drops the <hip/hip_bf16.h> include ordering workaround, which only existed because those macros used to rewrite the header's own declarations. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON -DPopSift_USE_TEST_CMD=ON \ -DPopSift_TESTFILE_PATH=<path to the Oxford datasets> cmake --build build-hip -j cmake --build build-hip --target run-test-boat ``` On a Radeon Pro W7800 (gfx1100) the six Oxford boat images give 8351/9874, 7946/9452, 6158/7280, 4802/5799, 4618/5476 and 3855/4618 features/descriptors, identical to the counts before this change, and img1 features.txt is md5-identical.
A descriptor whose norm is zero cannot come from a real extremum, so both normalizers now reject that case in one place instead of scaling by an infinite factor and repairing the result afterwards. L2 keeps the inverse norm at zero when the sum of squares is zero, and RootSift writes an all-zero descriptor when the bin sum is not positive. The per-bin test in RootSift replaces the previous clamp. It also covers a bin that came out marginally negative, which can happen where the weight accumulation has no round-toward-positive-infinity intrinsic and falls back to round-to-nearest, and keeps such a bin out of the square root. The shuffle width in both files is now passed explicitly on every platform, for the same reason as the surrounding kernels: the reduction and the lane-zero broadcast span one row of the (32,32) normalization block, which is one descriptor, and not the hardware warp. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON -DPopSift_USE_TEST_CMD=ON \ -DPopSift_TESTFILE_PATH=<path to the Oxford datasets> cmake --build build-hip -j cmake --build build-hip --target run-test-boat ``` The run-test-boat target uses --root-sift, so it exercises the RootSift path. On a Radeon Pro W7800 (gfx1100) the six Oxford boat images give the same counts as before this change and img1 features.txt is md5-identical, with no NaN or infinity in any descriptor.
The read handle a kernel takes for a pyramid array is now declared in src/popsift/sift_textures.h, together with LinearTexture, rather than inside common/assist.h, because the type appears in kernel signatures all over the library and is not an implementation detail of the assist helpers. The header is the place for the texture and surface handle types. Octave hands out the read handle itself. It knows the array, the texture, the surface and the level dimensions, so a call site no longer assembles a handle out of four accessors: it asks for the handle it wants, point filtered or linear filtered, over the blur data, over the intermediate data or over the DoG. The accessors that only returned the raw texture object are gone, since a caller that wants to read has a better one to use. The helper that fetches a single clamped texel is now called texFetchClamped: it emulates a point filtered texture fetch, including the clamping and the byte offset of the surface x coordinate, so its name should say texture rather than surface. It clamps with std::clamp. The manual bilinear interpolation uses fmaf, as it does elsewhere in the descriptor code, and the comments about hardware linear filtering now record that gfx1100 accepts what gfx90a rejects, which is why the software path stays in the shared build. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON -DPopSift_USE_TEST_CMD=ON \ -DPopSift_TESTFILE_PATH=<path to the Oxford datasets> cmake --build build-hip -j cmake --build build-hip --target run-test-boat ``` On a Radeon Pro W7800 (gfx1100) the six Oxford boat images give the same counts as before this change and img1 features.txt is md5-identical.
The compatibility header described ROCm/rocm-systems#6683 as a partial fix covering only the layered surface read. A controlled experiment on a standalone reproducer showed the opposite: the collapse comes from the write, which passed the layer index in the mipmap level slot, so every layer landed in the same slot, and once the write is corrected all three read paths (layered surface read, layered texture fetch and host copy) return the right per-layer data. That result was posted on ROCm/clr#275. Comment only. The pyramid arrays stay non-layered, because the fix is not in ROCm 7.2.x. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON -DPopSift_USE_TEST_CMD=ON \ -DPopSift_TESTFILE_PATH=<path to the Oxford datasets> cmake --build build-hip -j cmake --build build-hip --target run-test-boat ``` On a Radeon Pro W7800 (gfx1100) the six Oxford boat images give the same counts as before this change.
Grid filtering is the only Thrust user today, but the setup it needs is not specific to it: the stream-bound execution policy lives in a different namespace on each platform. Putting it in common/thrust_setup.h lets a second user pick it up without repeating the distinction. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON -DPopSift_USE_TEST_CMD=ON \ -DPopSift_TESTFILE_PATH=<path to the Oxford datasets> cmake --build build-hip -j cmake --build build-hip --target run-test-boat ``` Grid filtering is compiled in by default, so the Thrust translation unit is built. On a Radeon Pro W7800 (gfx1100) the six Oxford boat images give the same counts as before this change.
Both creations of a linear filtered texture reported a point texture in their error message, copied from the point filtered creation above them. The comment next to the point filtering fallback asked for the behaviour to be re-verified on RDNA. It has been: gfx1100 creates the hardware linear filtered texture that gfx90a rejects, so the fallback is per device and the software interpolation in readTex is what keeps one build correct on either of them. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx1100 \ -DCMAKE_HIP_COMPILER=/opt/rocm/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON -DPopSift_USE_TEST_CMD=ON \ -DPopSift_TESTFILE_PATH=<path to the Oxford datasets> cmake --build build-hip -j cmake --build build-hip --target run-test-boat ``` Message and comment text only. On a Radeon Pro W7800 (gfx1100) the six Oxford boat images give the same counts as before this change.
The commit that simplified the normalization guards reduced the RootSift test to a comparison of the bin sum against zero. That reopens an overflow this branch had already closed: a subnormal sum makes the reciprocal overflow to infinity, so a positive bin normalizes to infinity instead of to zero. Measured on an Instinct MI300X, with the test against zero a bin of 1e-41 with a sum of 1e-40 gives infinity, and with the threshold it gives zero. A sum that small is a degenerate window rather than a real descriptor, so treating it like an all-zero one is what the surrounding code already does for a sum of exactly zero. The threshold applies on both platforms now; before the simplification it was on the AMD path only, while the NVIDIA path divided by the sum per bin. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx90a \ -DCMAKE_HIP_COMPILER=<rocm>/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON -DPopSift_USE_TEST_CMD=ON \ -DPopSift_TESTFILE_PATH=<path to the Oxford datasets> cmake --build build-hip -j cmake --build build-hip --target run-test-boat ``` The run-test-boat target passes --root-sift, so it exercises this path. The threshold only changes the outcome for a bin sum below 1e-20, which no real image produces, so the Oxford boat feature and descriptor counts are unchanged. The NVIDIA path was compile-checked with nvcc 12.8 for sm_86.
The comment above the rocThrust lookup said that the Thrust code in the grid filter compiles unchanged on AMD. That was never accurate: the stream-bound execution policy is in a different namespace there, which is the reason common/thrust_setup.h exists. The comment now points at that header instead of claiming there is nothing to do. Comment only, no change to what is built. This work was done with the assistance of an AI coding agent. Test Plan: ``` cmake -S . -B build-hip -DUSE_HIP=ON -DCMAKE_HIP_ARCHITECTURES=gfx90a \ -DCMAKE_HIP_COMPILER=<rocm>/llvm/bin/clang++ -DCMAKE_BUILD_TYPE=Release \ -DPopSift_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=ON cmake --build build-hip -j ``` Grid filtering is on by default, so the Thrust translation unit is compiled.
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. |
jeffdaily
added a commit
to AMD-Ecosystem/moat
that referenced
this pull request
Aug 14, 2026
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
1 participant
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.
What the maintainer asked
griwodz left a changes-requested review on alicevision#186 (2026-08-07) with 36 inline comments. The substantive asks: pass every shuffle width explicitly instead of keeping
#ifdef USE_HIPpairs, route the shuffle compatibility throughPopSift_HAVE_SHFL_DOWN_SYNCin assist.h, gather the texture and surface type declarations in one header, have Octave return filled read handles instead of raw texture accessors, simplify the RootSift and L2 normalization guards, fix a copy-pasted error string and several stale comments, share the Thrust setup, and use fmaf in the manual bilinear lerp. The rest were informational (rdc stays, CUDA 10 / C++17 CMake modernization belongs in his alicevision#190, texel-center convention, a future flat-3D-memory redesign).What changed (f271272..d10126b, 8 commits, 28 files, +272/-436)
fe7135c[ROCm] Pass the shuffle width explicitly on every platformdf795a3[ROCm] Simplify the descriptor normalization guardsdbb157c[ROCm] Declare the texture and surface types in one header67bbce7[ROCm] Correct the description of the layered array collapsea0b95cc[ROCm] Move the Thrust setup into a shared header4d51a78[ROCm] Fix the linear texture error strings and filter comments6d7766d[ROCm] Keep the small threshold in the RootSift normalizationd10126b[ROCm] Correct the comment on the rocThrust dependencyThe per-comment disposition (implemented / skipped-with-reason for all 36 comments) is in
projects/popsift/notes.md, sections dated 2026-08-13 and 2026-08-14. The delta was reviewed in a third pass on windows-gfx1151: review-passed, no problem found.Two things the approver should weigh:
sum > 1e-20fRootSift gate as his removal request; it was not (it answered an earlier bot finding about subnormal-sum overflow in this same PR). The gate is restored at the tip, now on both platforms, and the reply presents it as our decision, with the reason. The commit body discloses the small behavioral change to the NVIDIA path (it previously divided each bin by the sum).What was re-tested at the staging tip (d10126b)
4d51a78(the remaining delta to the tip is one comment hunk plus the one-line threshold), windows-gfx1201 and windows-gfx1101 validated at the published tipf2712723.Upstream reply
Thanks for the careful review. All comments are addressed on the branch; here is a summary.
Every shuffle now passes its width explicitly, on both platforms, so the
#ifdef USE_HIPpairs around the shuffles are gone. The widths are the algorithmic group widths (a descriptor row, the bitonic network, block.x), which is why they are literal 32s rather than warpSize.The shuffle compatibility now goes through
PopSift_HAVE_SHFL_DOWN_SYNCas you suggested: it is 0 on the HIP build, so assist.h takes its existing pre-CUDA-9 branch, which is exactly the spelling HIP provides. The seven__*_syncmacros are gone from the compatibility header, and with them the bf16 include-order workaround they had caused.The texture and surface types now live in one header,
src/popsift/sift_textures.h(LayeredReadTex and LinearTexture), and the layered-source macro is replaced by amakeLayeredReadTex()helper there.Octave now returns filled read handles (
getDataReadTexPoint/Linear,getIntermReadTexPoint/Linear,getDogReadTexPoint) built from its own members, and all 22 launch sites use them. Since you wrote "instead of Octave::getDataTexPoint()", the five superseded raw accessors are removed; say the word if you want them kept and I will restore them.The RootSift normalization has the structure you wrote: the early
if (inv <= 0.0f)zeroing and the per-bin ternary withscalbnf. One note: the small-sum threshold (sum > 1e-20f) is kept, and now applies on both platforms. It was added earlier in this PR because a subnormal sum makes__fdividef(1.0f, sum)overflow to infinity; yourinv <= 0.0ftest is unaffected by it. The NVIDIA path previously divided each bin by the sum directly, so this is a tiny behavioral change on that path, called out in the commit message.The L2 normalization guard is one line on both paths:
norm = (norm > 0.0f) ? __frsqrt_rn(norm) : 0.0f.Also done: the copy-pasted "point texture" error string on the linear-texture creation, the stale comments (including recording that gfx1100 accepts hardware linear filtering, so the software bilinear is a per-device workaround kept so one binary serves both), a shared Thrust setup header (
common/thrust_setup.h), andfmafin the manual bilinear lerp (the compiler had already contracted it, so the binary is unchanged).One suggestion I did not take:
x - static_cast<int>(x)forx - floorf(x). The x there is not always positive: the horizontal filter readsreadTex(src, off_x - span, ...)and the gradient readsx - 1.0f, so x goes a few texels negative at the image border, where truncation and floor differ. floorf keeps the clamped-border behavior.On the other points: agreed that rdc stays; the CUDA 10 / C++17 CMake modernization makes sense in alicevision#190 rather than here; and replacing the texture path with flat 3D memory and a leaner Octave class reads like the right long-term direction, which this change deliberately does not start.
Since your review the branch was validated again end to end on an MI250X (gfx90a) and a Radeon 8060S (gfx1151, Windows), with earlier passes of the same changes on a Radeon Pro W7800 (gfx1100) and two more Windows machines (gfx1201 and gfx1101); the CUDA arm compile-checks clean with nvcc 12.8. On gfx90a one of the six benchmark images yields 9451 descriptors where the other architectures give 9452, stable across runs -- a single boundary keypoint on the 64-wide wavefront, noted here for completeness.