Skip to content

Keep the value's pointer type when the emitted storage class is logical - #1663

Open
paoloantinori wants to merge 5 commits into
google:mainfrom
paoloantinori:keep-logical-pointer-type
Open

paoloantinori wants to merge 5 commits into
google:mainfrom
paoloantinori:keep-logical-pointer-type

Conversation

@paoloantinori

Copy link
Copy Markdown
Contributor

When -physical-storage-buffers is on, getSPIRVPointerOperand reconciles a pointer value's emitted SPIR-V type with the type its consumer expects by emitting an OpBitcast to the expected type. If the value was emitted with a logical storage class, that OpBitcast has a logical pointer operand, which is invalid SPIR-V:

error: Instruction may not have a logical pointer operand
  %14916 = OpBitcast %_ptr_PhysicalStorageBuffer_uint %14915
  %14915 = OpAccessChain %_ptr_Private_uint %55 %14913

This surfaces on the hashcat kernel from #1660 once both the parent branch's rehome (optimizer lookup tables become Private variables while some GEP users keep stale Global-AS types) and physical addressing are active: 32 occurrences, module rejected by spirv-val.

The fix records the storage class of every OpTypePointer the producer registers (a small type_id -> storage_class map filled at type creation) and uses it at the mismatch site: when the emitted class is logical, keep the value's own pointer (loads and stores through it stay in its storage class, which is legal); when both sides are PhysicalStorageBuffer, keep emitting the reconciling bitcast that issue #1637 and the fix in #1639 depend on. So this preserves the existing physical-to-physical path exactly.

Verification, on this branch (parent #1662 plus this delta):

  • full preprocessed hashcat m22000 kernel, -arch=spir64 -physical-storage-buffers -O2: compiles and spirv-val --target-env vulkan1.2 is completely clean (previously the error above);
  • the same kernel with the logical (default) addressing and clvk's full option set: unchanged, still clean;
  • the existing test/PhysicalStorageBuffers/issue-1637.cl under the same physical options: still emits its physical-to-physical OpBitcast and validates clean;
  • the included regression test compiles a kernel that exercises both the rehome (dense switch at -O2) and the mismatch site under physical addressing, FileChecks that the table lands as a Private variable with its initializer and that no OpBitcast %_ptr_PhysicalStorageBuffer from a logical operand is emitted, and validates at vulkan1.2.

Based on #1662 because the invalid bitcast only becomes reachable once the rehome exists; if #1662 lands first this applies cleanly on top.

@google-cla

google-cla Bot commented Oct 8, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@paoloantinori
paoloantinori force-pushed the keep-logical-pointer-type branch 2 times, most recently from 6849d8b to b16a2ed Compare October 9, 2026 10:57
@alan-baker

Copy link
Copy Markdown
Collaborator

Please rebase this change now that #1662 is merged.

getSPIRVPointerOperand reconciles a pointer value's emitted SPIR-V
type with the type the consumer expects. Under
-physical-storage-buffers, when the types differ it emitted an
OpBitcast to the expected PhysicalStorageBuffer pointer type. If the
value was emitted with a logical storage class (for example a Private
variable produced by rehoming optimizer-generated tables, as in the
parent branch), that OpBitcast has a logical pointer operand, which
is invalid SPIR-V:

  error: Instruction may not have a logical pointer operand
    %14916 = OpBitcast %_ptr_PhysicalStorageBuffer_uint %14915
    %14915 = OpAccessChain %_ptr_Private_uint ...

Track the storage class of every registered SPIR-V pointer type in a
small registry filled at OpTypePointer creation, and use it to decide:
when the emitted class is logical, keep the value's own pointer
(loads and stores stay in its storage class, which is legal); when
both sides are PhysicalStorageBuffer, keep emitting the reconciling
bitcast that google#1637 depends on.

Verified on the full preprocessed hashcat m22000 kernel with
-arch=spir64 -physical-storage-buffers -O2: the module validates
clean at vulkan1.2 (previously the exact error above, 32 times). The
physical-to-physical bitcast path is unchanged: the existing
issue-1637 test still emits and validates its bitcast. The logical
build and the regression tests from the parent branch are unchanged.
@paoloantinori
paoloantinori force-pushed the keep-logical-pointer-type branch from b16a2ed to dc34181 Compare October 15, 2026 16:02
@paoloantinori

Copy link
Copy Markdown
Contributor Author

Rebased onto current main in dc34181. The branch now carries only the pointer-type fix and its regression test on top of the merged #1662. I verified on a local build of the rebased branch that the new test, the switch table test, and the issue-1637 test pass, and the full PointerCasts directory shows no new failures.

Separately, test/PointerCasts/issue-1187.ll fails on current main (975e3c8) for me with error: invalid LLVM IR input: domain must have one or two operands on the committed metadata. That looks like stale test data after an LLVM update rather than anything from these PRs.

Comment thread test/physical_bitcast_logical_pointer.cl Outdated
Comment thread lib/SPIRVProducerPass.cpp Outdated
getSPIRVPointerOperand now resolves rehoming with the existing
GetPointerAddressSpace helper and builds the expected pointer type from
the resolved address space, so rehomed values keep their Private type
and no spurious PhysicalStorageBuffer type is registered. The
PointerTypeClass registry is gone. Test trimmed to the switch that
triggers the invalid bitcast.
// RUN: spirv-val --target-env vulkan1.2 %t.spv

// CHECK: OpVariable %_ptr_Private__arr_uint_uint_4 Private %{{[0-9]+}}
// CHECK-NOT: OpBitcast %_ptr_PhysicalStorageBuffer %{{[0-9]+}}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because typed pointers are used here, spirv-dis prints %_ptr_PhysicalStorageBuffer_uint, so CHECK-NOT: OpBitcast %_ptr_PhysicalStorageBuffer %{{[0-9]+}} (with a space before %) won't actually match OpBitcast %_ptr_PhysicalStorageBuffer_uint %<id>.

Suggested change
// CHECK-NOT: OpBitcast %_ptr_PhysicalStorageBuffer %{{[0-9]+}}
// CHECK-NOT: OpBitcast

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, the check had no teeth. Applied in f708cab: the CHECK-NOT is now any OpBitcast. On this branch the disassembly contains zero OpBitcast at all, and the pre-fix output I captured from main yesterday (one OpBitcast with the typed pointer name) now fails the check as it should.

@paoloantinori
paoloantinori force-pushed the keep-logical-pointer-type branch from f708cab to 6b802dc Compare October 16, 2026 15:31
The typed-pointer name printed by spirv-dis is
%_ptr_PhysicalStorageBuffer_uint, so a pattern expecting a space and an
id after PhysicalStorageBuffer never matched and the check had no
teeth.
@rjodinchr
rjodinchr enabled auto-merge (squash) October 17, 2026 13:42
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.

4 participants