Repository navigation
Add tests and benchmarks for the gather/scatter rewrite - #3987
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
bc42224 to
09fdd0f
Compare
Pins the index width, scale, factored offsets, gather-to-load, coalescing and known-mask decisions of ImproveMemoryOps and GatherCoalescePass, the order of the memory passes and the inliner, the location of performance warnings in inline helpers, and the code and warnings of the ispc#304, ispc#2720, ispc#2778, ispc#2901, ispc#2308, ispc#2919 and ispc#2943 reproducers. Switching off each transformation of the two passes one at a time showed six that no lit test noticed and seven that only one test noticed; every pin fails with its transformation switched off. Correct but suboptimal output carries an issue-linked TODO stating the condition for changing the check. memory-access-fallback.ll and memory-access-failed-match.ll (XFAIL) cover the general-address fallback and unchanged IR after a rejected match for the ispc#3990 rewrite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rule key The four input arrays of the functional test harness (AOT and JIT) are now allocated next to an inaccessible page: vfloat and vdouble end right at a trailing guard page, vint and vint2 start right after a leading one, so a gather, scatter or vector access past the end or before the start of an input faults instead of passing silently. WebAssembly has no page protection and keeps plain aligned arrays. The new rule key '// rule: run|skip on addressing=32|64' selects tests by the addressing mode ispc uses: the last --addressing flag of the ispc flags or ISPC_ARGS on 64-bit architectures, 32 on 32-bit architectures and in JIT mode, 64 on xe64. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…apes 24 tests using the guard pages of the functional test harness: masked gathers, scatters, loads and stores whose inactive lanes point into the guard page after or before an input, foreach tails and coalescable gathers that end at the array boundary (gather-guard-1/2, scatter-guard-1/2, masked-load-guard, masked-store-guard, coalesce-guard-1/2, guard-neg-1/2); signed and unsigned 8- to 64-bit indices for every element type, negative indices and byte offsets of 2^31 or more in both addressing modes (gather-index-1..4, scatter-index-1, gather-index-overflow-1/2); duplicate scatter indices and loads across aliasing stores, an atomic and two bases (scatter-dup, gather-alias-1/2/3); and the shapes of discussion ispc#2943, ispc#2778 and records read through inline helpers (rgba8-2943, struct-copy-2778, vector-gather-helper). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
01_trivial/18_gather_scatter gathers and scatters int8, int32 and double elements through int32, uint32, int16 and uint16 index arrays, with partial masks and tails, and index patterns the compiler can see (broadcast of 4, reverse, stride 2 and 4). 03_complex/01..05 cover moments of AOS point records read packed, strided and indexed, 16-byte records read and written through large inline helpers, the RGBA8 kernel of discussion ispc#2943, the bilinear remap of discussion ispc#2919 and the byte struct copies of ispc#2778, each with a reference implementation of the same element size. The weekly dashboard shows them in the not gated section until they have a stable history. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Guard-page runtime tests and IR tests for the gather, scatter and vector load/store address gaps that remain after ispc#3985: lane 0 off with a signed offset that overflows, offsets that wrap at 2^32 between lanes, a signed constant split without nsw, a struct member at 2 GB through a varying pointer, new[] sign extending an unsigned count, an unfolded +2^31 displacement, nneg copied to split or rescaled operands, a nested-index sum of two signed-i32 byte offsets that reaches 2^31, an explicit int64 sum converted to a varying pointer, and the integer-to-pointer provenance loss of ispc#3983. Each XFAIL test runs only the configurations that fail today; address-gaps-ok.ispc, address-nested-sum.ispc and address-provenance.ispc run the passing ones as regression gates, with the kernels and drivers shared in address-gaps.inc and address-provenance.inc. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8e997f5 to
f670e9e
Compare
dbabokin
left a comment
There was a problem hiding this comment.
Adversarial review of f670e9e: no actionable correctness or regression defects found.
I reviewed this from four independent angles: shared harness/argument handling and portability; functional-test semantics, bounds and oracles; lit/FileCheck and expected-failure coverage; benchmark correctness, measurement boundaries and dashboard integration.
Validation using a fresh build of this revision with LLVM 23.1.2 on macOS ARM64:
cmake --build build-review --target check-all -j 8: 622 passed, 11 expected failures, 217 unsupported, no unexpected failures.- All 24 new functional tests checked on
neon-i32x4andgeneric-i1x64, with both addressing modes: 23 passed plus the intended addressing-only skip in each 32-bit run; all 24 passed in each 64-bit run. Native JIT: 23 passed plus the intended skip. - Full functional suite on
neon-i32x4/aarch64: 1,653 passed, 15 skipped out of 1,668, zero compilation or execution failures. - All six new benchmarks compiled and passed their C++ correctness checks on both targets: 502 cases total. Dashboard selection also matched the intended not-gated cases.
- Direct harness probes verified 64-byte alignment, valid boundary accesses and faults immediately beyond the protected boundary. Addressing-selection probes covered flag precedence, fixed-width architectures and JIT.
- Independently compiled and checked the standalone IR XFAILs to confirm they fail for their documented transformations, rather than an unrelated compilation error.
Nonblocking coverage suggestion: split independent runtime configurations into separate XFAIL tests. For example, in address-gap-nested-sum.ispc:9–18, the first failing execution prevents the --wrap-signed-int and -O0 runs from executing in CI. Separate tests would continuously exercise each configuration and produce an XPASS when one is fixed. The PR explicitly acknowledges this limitation and reports independent manual checks, so I am not treating it as a blocking defect.
Local coverage limits: Linux-only guard-page runtime reproducers, native x86/Windows execution and other LLVM versions were not independently rerun. The current PR CI is green, but the local results above do not replace that platform coverage.
Description
Tests and benchmarks for the gather/scatter work, ahead of rewriting
ImproveMemoryOpsandGatherCoalescePass(#3990). No compiler changes.Lit tests pinning today's codegen (
gather-index-width,gather-scale,gather-factored-offsets,gather-to-load,coalesce-gathers,masked-memops-known-mask,inline-helper-gather,perf-warn-inline-location,pipeline-order, plus the reproducers of gather coalescing doesn't happen for simple program #304, Perform full vector stores when sequentially setting uniform values in multiples of the native vector width #2720, Gather/Scatter can be avoided if Source and Destination are of same type. #2778, Codegen for varying bytes #2901, Unexpected scatter performance warnings using soa #2308, How to process H,W,C effectively? #2919 and How to work with interleaved RGBA data #2943). Each case pins today's output. Correct but suboptimal output carries an issue-linkedTODOcomment stating the condition under which the check may change, so the change that implements the improvement updates the check and removes the TODO.Lit tests for the transformations no test noticed. I switched off each transformation of the two passes, one at a time:
The new tests pin each of them. They also cover
--wrap-signed-int,--opt=force-aligned-memory, 32-bit x86, x64 coalescing, barriers in the coalescing window and 32-bit offset narrowing. Every pin was checked to pass on today's compiler and to fail with its transformation switched off.memory-access-fallback.llchecks that general varying addresses and unequal insert-chain bases keep the general fallback;memory-access-failed-match.ll(XFAIL) checks that a rejected base recovery leaves the IR unchanged, which step 1 of Refactor ImproveMemoryOps around shared, read-only address analysis #3990 requires.Guard pages in the functional test harness. The input arrays (AOT and JIT) now sit next to an inaccessible page, so an access past the end or before the start of an input faults instead of passing silently. A new rule key,
// rule: run|skip on addressing=32|64, selects tests by addressing mode.24 functional tests for footprint (inactive lanes and tails next to the guard pages), index semantics (8- to 64-bit signed and unsigned indices, byte offsets of 2^31 or more), conflicts (duplicate scatter indices, loads across aliasing stores, an atomic and two bases) and shapes (How to work with interleaved RGBA data #2943, Gather/Scatter can be avoided if Source and Destination are of same type. #2778, records read through inline helpers).
Gather/scatter microbenchmarks:
01_trivial/18_gather_scatterand03_complex/01..05. They show in the not-gated section of the dashboard until they have a stable history.XFAIL reproducers for the known address gaps that remain after Unsigned 32-bit gather index is scaled with nsw before zero-extension (poison for offsets above INT32_MAX) #3985. Guard-page runtime tests (
address-gap-*.ispc): a masked access whose lane 0 is off and whose signed offset overflows, offsets that wrap at 2^32 between lanes, a signed constant split withoutnsw, a struct member at 2 GB through a varying pointer, a nested-index sum of two signed-i32 byte offsets that reaches 2^31 without any integer-to-pointer cast (the counterexample of the Refactor ImproveMemoryOps around shared, read-only address analysis #3990 design doc), and the integer-to-pointer provenance loss of Memory builtins lose pointer provenance and capture info; redundant gathers and loads survive -O2 on all targets #3983. IR tests:newsign extending an unsigned count, an unfolded+2^31displacement that is not safe to truncate,nnegcopied to split or rescaled operands (address-gap-nneg-*.ll), and an explicit int64 sum converted to a pointer. Each XFAIL test runs only the configurations that fail today;address-gaps-ok.ispc,address-nested-sum.ispcandaddress-provenance.ispcrun the passing ones as regression gates. The kernels and drivers are shared inaddress-gaps.incandaddress-provenance.inc.Bugs found along the way and filed separately: #3983 (integer-to-pointer casts lose provenance), #3984 (GatherCoalescePass and negative offsets with
--addressing=32), #3985 (nswon unsigned index scaling), #3986 (crash after an undeclared symbol in a template).Testing
check-allpasses on the currentmain(ded7463) with LLVM 21.1.8: 711 passed, 18 expected failures, 121 unsupported, no failures. The new and changed lit tests were also run on LLVM 20, 22, 23 and trunk in earlier revisions of this branch.main: 1668 tests, no compilation or execution failures, 7 skipped (AMX, Xe and the--addressing=64-onlygather-index-overflow-2). Earlier revisions of this branch were run on sse4-i32x4, avx2-i32x8, avx2-i32x16, avx512skx-x16 and avx512skx-x64, with--jitand with--addressing=64.Related Issue
Checklist
clang-format(e.g.,clang-format -i src/ispc.cpp)🤖 Generated with Claude Code