Merge upstream develop to clear the PR #186 conflict - #2
Open
jeffdaily wants to merge 3 commits into
Open
Conversation
CUDA 13.3 no longer exposes these declarations transitively. Include the headers used by s_filtergrid.cu so thrust::tuple, thrust::make_tuple, and thrust::make_zip_iterator remain available. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merge pull request alicevision#188 from BillyONeal/fix/cuda-13.3-thrust-includes
Upstream added <thrust/iterator/zip_iterator.h> and <thrust/tuple.h> to src/popsift/s_filtergrid.cu because CUDA 13.3 stopped pulling those headers in transitively. This branch had already reworked the same include block, replacing <thrust/execution_policy.h> and <thrust/version.h> with common/thrust_setup.h, which centralizes the stream-bound Thrust execution policy that is spelled differently on each platform. That is the only file that conflicted. Resolved as the union of both intents: the include of common/thrust_setup.h and the surrounding block shape stay as they are here, and both new upstream headers are added in their sorted positions. They are specific to this translation unit, so they belong here rather than in the shared header. rocThrust provides both, so the AMD build is unaffected. 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_PATH/lib/llvm/bin/clang++ -DCMAKE_PREFIX_PATH=$ROCM_PATH \ -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 ```
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. |
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
3 participants
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.
Clears the merge conflict GitHub reported on upstream PR alicevision#186. No maintainer request in this round; nothing was asked for and nothing else changed.
Upstream commit
3d05ad0("Include Thrust tuple and zip iterator headers", merged todevelopas36d704don 2026-08-07) added#include <thrust/iterator/zip_iterator.h>and#include <thrust/tuple.h>tosrc/popsift/s_filtergrid.cu, because CUDA 13.3 stopped pulling them in transitively. Oura0b95cchad reworked the same include block to route throughcommon/thrust_setup.h. Both sides edited overlapping lines, so the PR went CONFLICTING.src/popsift/s_filtergrid.cuwas the only conflict; the rest ofb1c8199..36d704dmerges clean because that range touches no other file. Git auto-mergedzip_iterator.h; the textual conflict was the trailingtuple.h/version.hpair. The resolution keeps our include-block shape and adds upstream's two headers in their sorted positions.common/thrust_setup.hincludes only<thrust/execution_policy.h>and<thrust/version.h>, so neither new header duplicates it, and both are used by this translation unit alone, so they belong here rather than in the shared header. rocThrust ships both.Net delta against the published tip
d10126b5dab3is thereforesrc/popsift/s_filtergrid.cu, +2/-1 -- byte-identical to upstream's own text -- plus upstream's end-of-file blank-line removal that came in with the merge.Upstream reply
Merged
developto clear the conflict onsrc/popsift/s_filtergrid.cu. The overlap was your CUDA 13.3 include fix in alicevision#188 landing on the same include block this branch had reworked to route throughcommon/thrust_setup.h. Kept both: your two headers in their sorted positions, our shared-header include.thrust_setup.honly pulls inexecution_policy.handversion.h, so nothing is duplicated.Rebuilt and re-ran the correctness checks after the merge. The Oxford boat feature and descriptor counts are unchanged on every image, and the CUDA build is still clean with nvcc 12.8 (sm_86, 0 errors, 0 warnings).
Review notes (not published upstream)
Re-tested at
758d5e77faf0cc64b86b785d9b0981f324b2c1adon three platforms, all PASS with byte-identical Oxford boat counts against each platform's own prior figures:d10126brecordinglinux-gfx942remains non-viable for reasons unrelated to this round (no texture/image API; the same failure exists at the published tip). The wave64 gate is carried by gfx90a, wave32 by gfx1100, windows by gfx1151.The changed translation unit's device path was exercised directly, not just linked:
--filter-max-extrema 2000 --filter-grid 4runs a stable 2008 selected features across repeats on both gfx90a and gfx1151, no NaN, exit 0.CUDA no-regression gate re-run at this branch head rather than reused from the earlier tip, since this round's only content change is the CUDA include fix itself: nvcc 12.8.93, sm_86, clean from scratch, 29 objects including
s_filtergrid.cu.o, 0 errors, 0 warnings. That exercises nvcc 12.8, not the CUDA 13.3 floor that originally broke; the text is upstream's own, unmodified.jargon.py --port popsiftshows its one long-standing hit in05e698ec8, an ancestor of the published tip and live in alicevision#186 since it opened -- unchanged by this round.