Skip to content

Flatten nested insertvalues for Intel's SPIR-V driver - #953

Merged
maleadt merged 1 commit into
mainfrom
tb/spirv-intel-insertvalue
Sep 25, 2026
Merged

maleadt merged 1 commit into
mainfrom
tb/spirv-intel-insertvalue

Conversation

@maleadt

@maleadt maleadt commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Intel's graphics compiler (IGC) miscompiles aggregates built by nested insertvalue chains. Its TypesLegalizationPass splits aggregate stores into per-field stores by looking up each field in the chain. That lookup gives up on an insertvalue with more indices than the field it's looking for, so the store of that field is silently dropped. Aggregate phis are hit as well, because IGC lowers them to stores into an alloca.

For example, storing FlagFirst(flag, (a, b)), with struct FlagFirst; valid::Bool; value::Tuple{Float32,Int32}; end, loses flag. That breaks e.g. findmin-style reductions over (Bool, value) tuples. See JuliaGPU/OpenCL.jl#502, JuliaGPU/oneAPI.jl#259, and intel/intel-graphics-compiler#378. The bug has been in IGC since 2018 and is still on its master branch.

This PR adds a driver field to SPIRVCompilerTarget, so that consumers can say which driver will consume the SPIR-V. It defaults to :generic. When the driver is :intel, finish_ir! flattens multi-index insertvalues into single-index extract/insert pairs, which IGC handles correctly. The pass has to run after optimization, because InstCombine folds the pairs back together.

oneAPI.jl carries its own copy of this pass (from JuliaGPU/oneAPI.jl#548). It could pass driver=:intel and drop that copy.

The change is non-breaking: driver is a keyword argument with a default, and nothing changes unless a caller passes :intel. The @kwdef-generated positional constructor does gain an argument, but I'm not aware of anyone using it.

Tested on an Iris Xe (NEO 26.18, IGC 2.34) with OpenCL.jl passing driver=:intel. Without this change, the issue's reproducer, a nested struct, a loop-carried aggregate and oneAPI.jl#259's mapreduce all return wrong results. With it, they are all correct.

Fixes JuliaGPU/OpenCL.jl#502

Intel's graphics compiler silently drops the store of a field when legalizing an
aggregate that was built by a chain of insertvalues, if a later one in the chain has
more indices. Add a `driver` field to SPIRVCompilerTarget so that consumers can
identify the driver, and flatten nested insertvalues when it is `:intel`.
@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.46%. Comparing base (8496909) to head (1fe0ea9).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/spirv.jl 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #953      +/-   ##
==========================================
+ Coverage   86.41%   86.46%   +0.05%     
==========================================
  Files          29       29              
  Lines        5674     5703      +29     
==========================================
+ Hits         4903     4931      +28     
- Misses        771      772       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@maleadt
maleadt merged commit 2f56a83 into main Sep 25, 2026
32 of 33 checks passed
@maleadt
maleadt deleted the tb/spirv-intel-insertvalue branch September 25, 2026 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Intel's OpenCL driver (NEO) stores a wrong Bool field in a struct followed by a tuple

1 participant