Allow __getAddress on a mutable structured-buffer element (#12581) - #12592
Allow __getAddress on a mutable structured-buffer element (#12581)#12592nv-slang-bot[bot] wants to merge 11 commits into
__getAddress on a mutable structured-buffer element (#12581)#12592Conversation
|
Automated notice (PR board sync) — do not reply to this comment. Auto-assigned @jvepsalainen-nv as shepherd for this Bot PR. FYI for maintainers: committer signal on the changed files is higher for jkwak-work than for the auto-requested reviewer (jvepsalainen-nv). They were not auto-requested; a human may optionally add them as a reviewer. |
|
Automated notice (PR board sync) — do not reply to this comment. Auto-assigned @jvepsalainen-nv as shepherd for this Bot PR. FYI for maintainers: committer signal on the changed files is higher for jkwak-work than for the auto-requested reviewer (jvepsalainen-nv). They were not auto-requested; a human may optionally add them as a reviewer. |
A subscript into an RWStructuredBuffer/RasterizerOrderedStructuredBuffer is a device-memory lvalue, and &buf[i] already compiles, but __getAddress(buf[i]) was rejected with E31160. The AST address-of whitelist in getValidTypeForAddressOf only accepted a ref accessor with kIROp_GetOffsetPtr; extend it to also accept kIROp_RWStructuredBufferGetElementPtr when the base is a mutable structured buffer, returning Ptr<T, ReadWrite, Device, DefaultDataLayout> exactly as &buf[i] does. Read-only StructuredBuffer has no ref accessor and stays rejected.
4e5ed88 to
7bb0d5c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe address-of checker now accepts mutable structured-buffer and rasterizer-ordered buffer elements. New tests cover compilation, SPIR-V layout, runtime behavior, pointer arithmetic, dereference, subscripting, conversions, and immutable pointers. ChangesStructured-buffer element address support
Suggested reviewers: Merge Risk: 🔵 Low · up to The change is localized and adds coverage for mutable structured-buffer element addresses. It is mergeable with owner awareness that the accessor invariant should use a release assertion; otherwise, a future unsupported accessor reuse could produce an incorrect pointer type in release builds. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b606d0ce-cc15-44f8-b42f-9f6939ba1b75
📒 Files selected for processing (14)
source/slang/slang-check-expr.cpptests/diagnostics/invalid-constant-pointer-taking.slangtests/language-feature/pointer/get-address-buffer-element-accepted.slangtests/language-feature/pointer/get-address-buffer-element-ordered.slangtests/language-feature/pointer/get-address-buffer-element-runtime.slangtests/language-feature/pointer/get-address-buffer-element.slangtests/language-feature/pointer/get-address-validation.slangtests/language-feature/types/pointer-arithmetic.slangtests/language-feature/types/pointer-basic-operations.slangtests/language-feature/types/pointer-generic-syntax.slangtests/language-feature/types/pointer-immutable.slangtests/language-feature/types/pointer-subscript.slangtests/metal/pointer-in-buffer-getaddress.slangtests/spirv/get-address-buffer-element-layout.slang
💤 Files with no reviewable changes (1)
- tests/metal/pointer-in-buffer-getaddress.slang
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
| auto baseType = unwrapModifiedType(functionMemberExpr->baseExpression->type.type); | ||
| SLANG_ASSERT( | ||
| as<HLSLRWStructuredBufferType>(baseType) || | ||
| as<HLSLRasterizerOrderedStructuredBufferType>(baseType)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a release assertion for the accessor-type invariant.
Lines 7415-7417 describe an out-of-contract accessor reuse. SLANG_ASSERT is disabled in release builds. A future reuse of kIROp_RWStructuredBufferGetElementPtr could then return a read-write user pointer for an unsupported buffer type. Replace it with SLANG_RELEASE_ASSERT.
As per coding guidelines, use SLANG_RELEASE_ASSERT for out-of-contract input.
Source: Coding guidelines
| // Regression test for shader-slang/slang#12581: `__getAddress(buf[i])` on a mutable | ||
| // structured buffer must be accepted (the element is a device-memory lvalue, and | ||
| // `&buf[i]` already compiled). Exercises reading, arithmetic, and writing through it. | ||
|
|
||
| //TEST(compute):COMPARE_COMPUTE(filecheck-buffer=CHECK):-cpu -shaderobj -output-using-type | ||
| //TEST(compute):COMPARE_COMPUTE(filecheck-buffer=CHECK):-vk -emit-spirv-directly -output-using-type |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Put the //TEST directive first in each test file.
The coding guidelines require leading test directives. Move each //TEST directive above the narrative comments.
tests/language-feature/pointer/get-address-buffer-element.slang#L1-L6: Move the directives on lines 5-6 to the start of the file.tests/language-feature/pointer/get-address-buffer-element-runtime.slang#L1-L9: Move the directives on lines 8-9 to the start of the file.tests/language-feature/pointer/get-address-buffer-element-ordered.slang#L1-L7: Move the directive on line 7 to the start of the file.tests/language-feature/types/pointer-arithmetic.slang#L1-L5: Move the directives on lines 4-5 to the start of the file.tests/language-feature/types/pointer-subscript.slang#L1-L5: Move the directives on lines 4-5 to the start of the file.tests/language-feature/types/pointer-basic-operations.slang#L1-L5: Move the directives on lines 4-5 to the start of the file.tests/language-feature/types/pointer-generic-syntax.slang#L1-L5: Move the directives on lines 4-5 to the start of the file.tests/language-feature/types/pointer-immutable.slang#L1-L6: Move the directive on line 6 to the start of the file.
📍 Affects 8 files
tests/language-feature/pointer/get-address-buffer-element.slang#L1-L6(this comment)tests/language-feature/pointer/get-address-buffer-element-runtime.slang#L1-L9tests/language-feature/pointer/get-address-buffer-element-ordered.slang#L1-L7tests/language-feature/types/pointer-arithmetic.slang#L1-L5tests/language-feature/types/pointer-subscript.slang#L1-L5tests/language-feature/types/pointer-basic-operations.slang#L1-L5tests/language-feature/types/pointer-generic-syntax.slang#L1-L5tests/language-feature/types/pointer-immutable.slang#L1-L6
Source: Coding guidelines
|
[Supervisor] This chain's session hit its cost ceiling and is paused pending a human cost decision (dashboard → Continue/Stop). No action needed here — it resumes automatically once decided. |
|
[agent] CI is exposing a real SPIR-V lowering/address-space bug here, not a flaky GPU failure. The checker now gives As a result, code such as: int* ptr = __getAddress(dataBuffer[0]);tries to store a The seven new Vulkan pointer tests reproduce this consistently on Linux debug/release and Windows. The later “malformed HTTP or JSON response” messages are secondary: the invalid shader is rejected and the test server fails to return a valid response; the harness reproduces it on a fresh server and attributes it to the input. The CPU-only job passes because this address-space distinction does not affect the CPU lowering path. This means the checker-only approach is insufficient, and the claim that no lowering/IR work is needed does not hold. The fix needs to make the AST pointer type, Separately, the Falcor check is unrelated infrastructure noise: its external-CI trigger receives HTTP 403. |
|
Confirmed — thanks for the precise diagnosis; it holds on every point. I reproduced the exact To pin down where the disagreement actually lives:
Approach (no logical↔physical bitcast): reconcile in the address-space specialization pass — a local pointer slot / debug variable adopts the address space of the pointer value stored into it (specialize the pointer through its uses), rewriting the slot's contained pointer value-type I'll push the update and re-run CI. (Agreed the Falcor 403 is unrelated infra noise.) 🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
|
Thanks — the proposed address-space-specialization direction is the right one. I checked the SPIR-V debug emitter as well. Once the pointer value held by the Please make the change a general pointer-value dataflow fix rather than a
Suggested regression coverage:
After the update, please run the seven currently failing Vulkan variants locally with SPIR-V validation before restarting full CI. The PR is also behind |
…e in local slots `__getAddress(buf[i])` / `&buf[i]` on a mutable structured buffer has the surface type `Ptr<T, Device>` (matching the user's `int*`), but `RWStructuredBufferGetElementPtr` produces a descriptor-backed, logical `StorageBuffer` pointer. A local pointer slot that survives (a `-O0` local, or a `-g` debug backing variable) kept the physical `Device` pointee even though the value stored into it is a `StorageBuffer` pointer, so the store's pointer and object storage classes disagreed and SPIR-V validation failed with "OpStore Pointer type does not match Object type". Reconcile in the address-space specialization pass: a local pointer slot (`Var`/`DebugVar`) adopts the address space of the pointer values written into it, rewriting only the slot's contained pointer value-type (the outer `Function` storage class is unchanged) and propagating the specialized type through the slot's loads and their derived pointer ops. Writes are joined; a genuine conflict between two concrete address spaces is diagnosed as `InconsistentPointerAddressSpace` (also covering the pre-existing phi-argument TODO) rather than silently coerced. No logical<->physical bitcast is introduced. Also promotes the structured-buffer address-of type-gate assert in the checker to `SLANG_RELEASE_ASSERT`.
|
|
Reconcile local pointer slots to a fixpoint instead of a single ordered scan, so a slot fed from another slot resolves to that source slot's reconciled address space rather than its stale surface-default pointee, independent of the order slots are visited. A new `getStoredValueAddrSpace` resolves a stored value that is a load of a local pointer slot to the slot's reconciled space (`Generic` until reconciled), and resolves an address- computation op (`GetOffsetPtr`/`GetElementPtr`/`FieldAddress`) through to its base -- the derived inst's own type is not retyped until `propagateAddressSpaceFromInsts` runs after this pre-pass. Without this, e.g. `s + 1` with `s` a surviving slot is classified from a stale `Device` type and leaves the destination slot -- most visibly a `-g` `DebugVar` -- with a physical pointee while the value stored is a StorageBuffer pointer, failing SPIR-V validation with `OpStore ... type does not match Object`. A pointer bitcast is deliberately not resolved through: it reinterprets its operand and its declared result type is authoritative (an int-to-pointer physical pointer must stay physical). Interleave slot reconciliation and `propagateAddressSpaceFromInsts` in an outer fixpoint: reconciling slots retypes their loads; propagating those loads may retype the derived ops and block parameters that feed *other* slots, which are then reconciled (e.g. a pointer flowing through a block parameter and stored back into a slot). Both steps only move address spaces toward concrete -- each slot refines a bounded number of times, each inst is retyped once -- so the combined loop terminates. Fold the phi address-space join and conflict check into one `resolvePhiAddrSpace` helper called when a block parameter is first resolved and when it is revisited; on revisit, adopt a newly-consistent concrete join so a stale mapping is corrected, and diagnose each conflicting slot or block parameter exactly once. (The direct-SPIR-V pipeline eliminates phis before this pass; the phi path serves the pass's other, pre-phi-elimination callers.) Extend the tests to cover a pointer derived from a surviving slot, a control-flow-merged pointer stored into a further slot, and a `-O0` run of the conflict case alongside the default-optimization run.
… the reconcile pre-pass, tighten comments Review round 2 on #12592: - get-address-buffer-element-debug.slang: the positive `OpTypePointer StorageBuffer %int` checks passed with or without the fix (the element value is always a StorageBuffer pointer; the bug is the *slot's* contained type staying physical). Assert the physical storage class is absent instead (`CHECK-NOT: PhysicalStorageBuffer` / `CHECKDBG-NOT:`); this file takes no integer-to-pointer cast, so a correct compile emits no physical pointer, and the buggy `_ptr_Function__ptr_PhysicalStorageBuffer_int` slot type trips it. Fix the inaccurate comment claiming `-g` embedded source would self-match: slang-test removes embedded source for SPIR-V targets before FileCheck. - Scope the reconcile pre-pass to the SPIR-V (sink-carrying) path. It ran unconditionally for all four `specializeAddressSpace` callers, silently changing Metal/WGSL/GLSL slot types with no conflict diagnostic (no sink) and no test coverage. Gate it on `sink`, leaving those backends unchanged, and document the new `sink` parameter in the header. - getStoredValueAddrSpace: treat a not-yet-resolved `kIROp_Param` like a load of an unreconciled slot (return `Generic`) so a slot fed both a reconciled slot-load and a pointer phi does not compute a spurious conflict; convergence no longer depends on phi-elimination order. - resolvePhiAddrSpace: drop the sink-gated phi-conflict diagnose. It was unreachable (phis are eliminated before this pass on the only sink-carrying path) and duplicated `SPIRVLegalizationContext::processParam`, which owns phi-conflict diagnosis while phis exist. Keep the join. - getValidTypeForAddressOf: assert the two ref-accessor intrinsic-op flags are mutually exclusive so the ordered checks read as exhaustive rather than priority-dependent; correct the two comments (`__getAddress` is not unconditionally equivalent to `&`; the element is a logical StorageBuffer pointer, only typed `Device` at the AST level to match `&buf[i]`). - Consistency: uint3 SV_DispatchThreadID in the accept test; trim in-source termination proofs (they live in the PR process report).
A function parameter carries an authoritative declared address space, so when reconciling a local pointer slot's contained address space, getStoredValueAddrSpace must honor a stored function parameter rather than treat every kIROp_Param as an unresolved phi. Dropping a physical int* function parameter to Generic let a slot also fed a StorageBuffer element pointer merge silently, emitting an invalid OpStore. Only a phi (non-entry block parameter) is treated as unknown until the fixpoint resolves it. Add a regression test for the function-parameter conflict path, extend the CHECK-NOT coverage in the debug test, correct the header doc for the slot diagnostic, and rewrite change-history comments as durable rationale.
A function parameter's contained address space is not settled when the reconcile pre-pass runs: specializeFunc rewrites pointer parameters from their actual argument address spaces when callers are specialized, which runs after this pre-pass, so the declared int*->Device surface pointee is only provisional. Reading it wrongly rejects valid code -- a [noinline] helper whose int* parameter is passed a descriptor-backed StorageBuffer element pointer is specialized to StorageBuffer, so merging it with another element pointer is consistent. Treat any unresolved parameter (phi or function parameter) as Generic and let the slot's concrete writes drive reconciliation; a genuine conflict through a physical parameter surfaces downstream (post-specialization reconciliation is deferred, #13039). Replace the function-parameter conflict test with the acceptance test that pins the valid merge compiles, and correct the reconciled-conflict member comment (block-parameter conflicts are diagnosed by processParam).
Comment- and test-doc-only. The pre-pass does not reason about a pointer parameter's address space (it is settled later by call specialization), so cross-function parameter cases -- a physical-parameter conflict, and a specialized parameter's -g debug backing variable -- are out of scope and deferred to #13039. Because SPIR-V validation is opt-in, those cases can currently emit ill-typed SPIR-V caught only under validation, so drop the inaccurate 'always a compile error' framing. Note the acceptance test runs non-debug only and why.
Motivation
Taking the address of a mutable structured-buffer element with
__getAddresswas rejected on every target:The buffer element is a genuine device-memory l-value, not a function-local. The evidence that this is a gap rather than an intended prohibition:
&dataBuffer[0]already compiles today on the same buffer, and #10280 established that__getAddress(x)should be equivalent to&x. So the two spellings that are meant to be identical diverged for structured-buffer elements.Lifting that front-end rejection then exposed a latent SPIR-V lowering bug that CI caught on the
-vktargets: the resultingint*slot kept a physical (Device/PhysicalStorageBuffer) pointee even though the value written into it is a descriptor-backed logicalStorageBufferpointer, and a surviving slot (a-O0local, or a-gdebug backing variable) produced a store whose pointer and object storage classes disagreed:This PR therefore has two parts: (1) the front-end change that lifts E31160, and (2) the address-space fix that keeps the pointer logically
StorageBufferend-to-end so validation passes.Proposed solution
Part 1 — front-end (
getValidTypeForAddressOf).__getAddressand&route through different front-end paths.&binds through the normal__refmachinery;__getAddressparses to anAddressOfExprand is validated bygetValidTypeForAddressOf(source/slang/slang-check-expr.cpp). A subscriptbuf[i]desugars to anInvokeExprof the buffer'sref __subscript, whose intrinsic op iskIROp_RWStructuredBufferGetElementPtr(hlsl.meta.slang). TheInvokeExprbranch ofgetValidTypeForAddressOfonly whitelisted a ref accessor withkIROp_GetOffsetPtr, so it returnednullptrand the checker raised E31160.The fix extends that branch to also accept
kIROp_RWStructuredBufferGetElementPtr, returning the same pointer type&buf[i]produces:Ptr<T, ReadWrite, Device, DefaultDataLayout>. That intrinsic op is attached to a subscriptrefaccessor by exactly one place in the core module — thekMutableStructuredBufferCasestemplate inhlsl.meta.slang, which emits it only forRWStructuredBufferandRasterizerOrderedStructuredBuffer— so reaching this branch already implies one of those two types. The codeSLANG_RELEASE_ASSERTs that invariant (afterunwrapModifiedType, sogloballycoherentetc. are handled) rather than silently rejecting, per the codebase's "assert impossible shapes" rule.Part 2 — address-space specialization (
slang-ir-specialize-address-space.cpp). The surface typeint*defaults toAddressSpace.Device(== internalUserPointer== SPIR-VPhysicalStorageBuffer), soint* p = &buf[i]lowers to aPtr<Ptr<T, Device>, Function>slot. But the value written isRWStructuredBufferGetElementPtr, a logicalStorageBufferpointer, and there is no valid logical↔physical pointer conversion. When the slot is mem2reg'd to SSA the disagreement is invisible (only the value survives); a slot that must materialize — most commonly aDebugVarunder-g, or any-O0local — emits the mismatched store.The principled fix reconciles the slot with what is actually written into it: a local pointer slot (
Var/DebugVar) adopts the address space of the pointer values stored into it, rewriting only the slot's contained pointer value-type (the slot's ownFunctionstorage class is unchanged). This runs as a pre-pass in the address-space specialization pass, and the specialized type is then propagated through the slot's loads and their derived pointer users (GetOffsetPtr/GetElementPtr/FieldAddress, phi/block parameters) by the pass's existingpropagateAddressSpaceFromInstsmachinery — so a value that flows through control-flow re-assignment (if (c) p = q;) or a derived op (p[2]) stays logicalStorageBuffertoo. The address spaces of all writes into a slot are joined; a genuine conflict between two concrete address spaces (a slot fed both aStorageBufferand a physicalDevicepointer) is diagnosed asinconsistent-pointer-address-spacerather than silently coerced — the same diagnostic the pre-existing phi-argument TODO inprocessFunctionwas always meant to raise, which this change also now wires up. No logical↔physical bitcast is introduced anywhere.This is the correct layer: the descriptor-backed
StorageBufferpointer produced byRWStructuredBufferGetElementPtris the canonical, intended representation; the staleDevicepointee on the slot is the accidental artifact of the surfacePtr<T>default, and the address-space specialization pass already exists to resolve exactly theseGeneric/under-specified pointer types before emission. Fixing it there (rather than guarding in the SPIR-V emitter) keeps emission simple, per the codebase philosophy.Change summary
source/slang/slang-check-expr.cppInvokeExprbranch ofgetValidTypeForAddressOfto accept a mutable structured-buffer subscript (kIROp_RWStructuredBufferGetElementPtr), returningPtr<T, ReadWrite, Device, DefaultDataLayout>, andSLANG_RELEASE_ASSERTthe base isRWStructuredBuffer/RasterizerOrderedStructuredBuffer(the intrinsic op guarantees it).source/slang/slang-ir-specialize-address-space.cppreconcilePointerSlots/reconcilePointerSlotWithStoredValue), gated to the SPIR-V / sink-carrying path, that specializes a local pointer slot's contained value-type to the joined address space of the pointer values written into it — resolving a stored load-of-a-slot or an address-computation op (GetOffsetPtr/GetElementPtr/FieldAddress) through to its base viagetStoredValueAddrSpace(a not-yet-resolved parameter — phi or function parameter — resolves toGeneric, since its address space is fixed only later by propagation or call-site specialization) — seedspropagateAddressSpaceFromInstswith the retyped loads, and diagnoses a genuine slot conflict once.resolvePhiAddrSpacejoins phi arguments across the fixpoint (phi-conflict diagnosis is owned byprocessParamin spirv-legalize, before phis are eliminated).source/slang/slang-ir-specialize-address-space.hDiagnosticSink*throughspecializeAddressSpace(forward-declared).source/slang/slang-ir-spirv-legalize.cppm_sinkintospecializeAddressSpaceso the pass can diagnose.tests/spirv/get-address-buffer-element-debug.slang-O0and-g2 -O0) for both__getAddress(buf[i])and&buf[i], with control-flow re-assignment and a derivedp[2]op; the-g2run is the exact shape CI originally caught.tests/diagnostics/pointer-address-space-conflict.slangStorageBufferelement pointer and a physicalDevicepointer across control flow is rejected withinconsistent-pointer-address-space.tests/spirv/get-address-buffer-element-param-merge.slang-O0, non-debug): a buffer-element pointer passed through a[noinline]helper'sint*parameter and merged with another element pointer must not be falsely flagged as an address-space conflict — pins that the pre-pass treats an unresolved parameter asGenericrather than reading its provisional physical type.tests/language-feature/pointer/get-address-buffer-element.slang-cpuand-vk): read, pointer arithmetic, and write through__getAddress(buf[i]).tests/language-feature/pointer/get-address-buffer-element-accepted.slangtests/language-feature/pointer/get-address-buffer-element-runtime.slang-cpu/-vkruntime test: member-of-element address read + write-back, and cross-element pointer arithmetic on a struct-element buffer (exercises element stride, not field size).tests/language-feature/pointer/get-address-buffer-element-ordered.slang-vkruntime test for aRasterizerOrderedStructuredBufferelement address (ROV is not accepted by nvrtc/CPU; it lowers to a storage buffer on Vulkan).tests/spirv/get-address-buffer-element-layout.slangStd140struct-element buffer carriesArrayStride 16— i.e.__getAddresspreserves the buffer's stride, matching&buf[i].tests/language-feature/types/pointer-{basic-operations,arithmetic,generic-syntax,immutable,subscript}.slang__getAddress(buf[i])(deref forms, arithmetic,Ptr<T>/T*equivalence,ImmutablePtr, subscript) on-cpuand-vk(theImmutablePtrone is-cpu-only, see the process report). These correspond to tests previously carried but disabled with an inaccurate "requires GPU runtime" tag — the real blocker was this front-end rejection.tests/language-feature/pointer/get-address-validation.slangRWStructuredBuffercase from a pinned rejection to an accepted use.tests/diagnostics/invalid-constant-pointer-taking.slangRWStructuredBufferE31160 expectation; keep the read-onlyStructuredBuffernegative.tests/metal/pointer-in-buffer-getaddress.slangConcepts and vocabulary
getValidTypeForAddressOf— the AST-level check that decides whether an__getAddress/AddressOfExprtarget is an addressable memory location and, if so, what pointer type it yields.kIROp_RWStructuredBufferGetElementPtr— arefsubscript accessor lowers to an intrinsic op; a mutable structured buffer's subscript uses this op to form an element pointer. Read-onlyStructuredBufferhas only agetaccessor (noref), so it never enters this branch.AddressSpace.Device/UserPointervsStorageBuffer— the Slang-surfaceDeviceaddress space maps to the internalAddressSpace::UserPointer, emitted as SPIR-VPhysicalStorageBuffer(a physical pointer: a raw device address). A descriptor-backed structured-buffer element pointer is instead a logicalStorageBufferpointer (an access chain into a bound resource). The two are not inter-convertible; a value cannot be both.Var/DebugVarof typePtr<Ptr<T, AS>, Function>is the slot (aFunction-storage local holding a pointer); thePtr<T, AS>it contains is the value. Part 2 rewrites only the contained value-type, never the outerFunctionslot.slang-ir-specialize-address-space.cpp, run during SPIR-V legalization, resolves under-specified (Generic) pointer address spaces by dataflow before emission.DebugVar— the IR node that carries a source variable for-gdebug info; it mirrors a realVarand, on SPIR-V, can emit a backingOpVariablewhose stored value must have a matching storage class.Process report
Part 1 — the whitelist extension (
slang-check-expr.cpp)The input shape handled here — an
InvokeExprof aref __subscriptwhose intrinsic op iskIROp_RWStructuredBufferGetElementPtr— is the canonical, intended representation ofbuf[i]for a mutable structured buffer (produced by the subscript→invoke rewrite againsthlsl.meta.slang'srefaccessor), not an accidental spelling. So this is the correct layer: it is the same node the existingkIROp_GetOffsetPtrcase already handles, and the producer is correct. That intrinsic op is attached to a subscriptrefaccessor by exactly one place — thekMutableStructuredBufferCasestemplate — which generates it forRWStructuredBufferandRasterizerOrderedStructuredBufferand nothing else (read-onlyStructuredBufferhas aget-only subscript, so it never reaches this branch). Reaching the branch therefore already implies one of the two mutable types; rather than silently rejecting a shape that cannot occur, the codeSLANG_RELEASE_ASSERTs that invariant (afterunwrapModifiedType, so a modified form likegloballycoherent RWStructuredBufferis handled), per the codebase's "assert impossible shapes rather than silently return a default" rule — a future accessor reusing this op then surfaces instead of producing a spurious E31160. Both mutable types are enabled deliberately (see below).Why the returned layout is
DefaultDataLayout, not the buffer'sL. The binding contract (issue + #10280) is that__getAddress(buf[i])must be identical to&buf[i]. Measured against the current compiler,&buf430[0]on aRWStructuredBuffer<int, Std430DataLayout>producesPtr<int, ReadWrite, Device, DefaultDataLayout>—&does not preserve the buffer'sL; it defaults it. Producing the same type is required by the "one canonical representation per value" rule: if this fix instead preservedL, thenint* p = __getAddress(buf430[0])would be a type mismatch (Ptr<…, Std430>vsint*=Ptr<…, Default>) and would fail to compile while&buf430[0]compiles — a regression the fix would introduce, and a divergence from the very equivalence it is meant to restore. (Preserving a genericLwould additionally hit aSLANG_RELEASE_ASSERTinASTBuilder::getPtrType, which reconstructs theIBufferDataLayoutwitness by walking the layout decl's constraints and cannot handle a generic type parameter.) Element stride is unaffected: the offset is computed from the buffer type's ownLat IR generation via theRWStructuredBufferGetElementPtrop, independent of the AST pointer's layout annotation.RasterizerOrderedStructuredBuffer decision. Enabled deliberately, not by accident:
&raster[i]already compiles today, and the equivalence contract makes__getAddress(raster[i])follow. It is compile-checked inget-address-buffer-element-accepted.slangand runtime-checked on Vulkan inget-address-buffer-element-ordered.slang(ROV is not accepted by nvrtc/CPU, so that runtime test is Vulkan-only).Part 2 — address-space specialization (
slang-ir-specialize-address-space.cpp,.h,slang-ir-spirv-legalize.cpp)The bug, concretely. Consider:
After Part 1, the checker types
pasint*=Ptr<int, Device>, so lowering builds a slotVar : Ptr<Ptr<int, Device>, Function>and aStore(slot, RWStructuredBufferGetElementPtr(...)). The stored value's real type isPtr<int, StorageBuffer>. With mem2reg the slot vanishes and only the value survives, so-O0-off builds validated by luck. But under-O0the slot survives, and under-g2anIRDebugVarbacking variable is materialized; the SPIR-V emitter then emitsOpStore %slot %valuewhere%slot's pointee isPhysicalStorageBuffer-classed and%valueisStorageBuffer-classed →OpStore Pointer type does not match Object type.The fix and its code trace.
reconcilePointerSlots()(called as a pre-pass at the top ofprocessModule, before the existing fixpoint loop) iterates to a fixpoint over everyVar/DebugVar, callingreconcilePointerSlotWithStoredValue(slot, reconciledLoads):StorewhosegetPtr()is the slot (and eachDebugValuewhosegetDebugVar()is the slot) it reads the stored pointer value's concrete address space viagetStoredValueAddrSpace, and joins them: the first concrete space wins; a second, different concrete space sets aconflictflag.getStoredValueAddrSpaceis the key to order-independence. For a load of another local pointer slot it returns that source slot's reconciled space (from areconciledSlotAddrSpacemap,Genericuntil the source slot is reconciled) — never the load's stale surface-default pointee. For an address-computation op derived from another pointer (GetOffsetPtr/GetElementPtr/FieldAddress) it resolves through to the base operand, because the derived inst's own type is not retyped until propagation runs after this pre-pass. A pointer-to-pointerBitCastis deliberately not resolved through: it reinterprets its operand, so its declared result type is authoritative. An integer-to-pointer cast is a different op (kIROp_CastIntToPtr, e.g. the conflict test's(int*)0x1000ull, which emitsOpConvertUToPtr) — it is not in the resolve-through set either, so it uses its own declared physicalDevicetype and is still detected as a conflict against aStorageBufferwrite. Any other value uses its own resolved pointer-type space.conflict, it emitsDiagnostics::InconsistentPointerAddressSpace(gated to the realVar, not the mirroredDebugVar, and de-duplicated via adiagnosedAddrSpaceConflictsset so the fixpoint reports each once) and returns without rewriting — the program is genuinely ill-formed and a debug slot must never be the thing that rejects it.reconciledSlotAddrSpace(unblocking any slot fed from this one), and if that space differs from the slot's declared contained space it rebuilds the contained pointer type with the new space and re-types the slot viagetPtrTypeWithAddressSpace— only the contained value-type changes; the outerFunctionstorage class is preserved. It then re-types everyLoadof the slot to the new contained type and records those loads inreconciledLoads.Because a slot fed from another slot only resolves after that source slot has, the slot scan is an inner
while (changed)fixpoint; the collected loads then seedpropagateAddressSpaceFromInsts— the pass's existing propagation worklist — so the specializedStorageBufferspace also flows to the loads' derived users (GetOffsetPtr/GetElementPtr/FieldAddresssuch as thep[2]in the test, and phi/block parameters), which were lowered with the staleDevicetype and are otherwise pinned to it by theaddrSpaceFromType != Genericearly-out inprocessFunction. The whole thing is an outer fixpoint: propagation may retype a derived op or block parameter that feeds yet another slot (e.g. a pointer flowing through a block parameter and stored back into a slot), so reconciliation re-runs until neither step makes progress. Both steps only move address spaces toward a concrete value — each slot refines a bounded number of times, each inst is retyped once — so the loop terminates.Concrete cascade the inner fixpoint handles. Consider
int* s = &buf[0]; int* alt = &buf[9]; if (c) s = alt; int* d = s + 1; *d;compiled with-g2. Here the conditional reassignment keepssas a surviving slot, andd's debug value isGetOffsetPtr(Load(s_slot), 1). Within the inner slot fixpoint, oncesreconciles toStorageBuffer(its writes are descriptor element pointers) and its loads are retyped, then whend's slot is scanned — in the same pass ifswas visited first, otherwise on the next inner pass —getStoredValueAddrSpaceresolves the storedGetOffsetPtrthrough its baseLoad(s_slot)tos's recordedStorageBuffer(viareconciledSlotAddrSpace) — no propagation round is needed, because the resolution reads the source slot's recorded space, not the derived inst's type. Sod(and itsDebugVar) specialize in the same fixpoint. Reading theGetOffsetPtr's own type instead — as a single ordered scan would — leavesd'sDebugVarphysical while the value isStorageBuffer, and-g2fails validation withOpStore ... type does not match Object. This exact shape is the derived-pointer regression inget-address-buffer-element-debug.slang. The outer reconcile↔propagate fixpoint is instead what covers a value whose type must first be changed by propagation before it can feed a slot — e.g. a live block parameter retyped bypropagateAddressSpaceFromInstsand then stored into a further slot, on the pass's pre-phi-elimination callers.Why a pre-pass, and why this layer. The address-space specialization pass is precisely the phase that resolves under-specified pointer address spaces before SPIR-V emission; the descriptor-backed
StorageBufferpointer is the canonical value and the slot'sDevicepointee is the accidental artifact ofPtr<T>'s surface default. So the producer of the "wrong shape" is the surface-type default, and the right place to reconcile it is this pass — not a guard in the emitter (which the codebase philosophy explicitly rejects: keep emission simple, do transforms in IR).The conflict diagnostic (input-shape check). A single pointer slot holding two different concrete address spaces is genuinely out-of-contract input — a SPIR-V pointer has exactly one storage class and there is no conversion between logical
StorageBufferand physicalPhysicalStorageBuffer— so the correct response is to fail loudly, not to silently pick one arm. The slot pre-pass (reconcilePointerSlotWithStoredValue) reportsinconsistent-pointer-address-spacewhen aVaris fed two different concrete spaces;tests/diagnostics/pointer-address-space-conflict.slangcovers it at both optimization levels (non-exhaustivemode, because the diagnostic is raised during SPIR-V legalization after source locations have been dropped, so it carries no line to annotate — precedent:custom-differential-type-error.slang).The pre-existing phi-argument conflict TODO in
processFunctionis addressed by explaining why it is unreachable here rather than adding a second diagnosis:SPIRVLegalizationContext::processParam(slang-ir-spirv-legalize.cpp) already diagnoses a pointer phi that merges two concrete address spaces, and it runs while phis still exist; the direct-SPIR-V pipeline then runseliminatePhisbefore this pass, so a conflicting phi never reachesspecializeAddressSpaceon the only path that carries a sink.resolvePhiAddrSpacetherefore only joins phi arguments (to map the phi's address space across the fixpoint) and deliberately does not diagnose — doing so would be dead on every path and would double-report withprocessParam. Relatedly,getStoredValueAddrSpacetreats an unresolved pointerkIROp_Param— whether a phi (non-entry block parameter) or a function parameter (entry-block parameter) — asGeneric. Neither has a settled address space at this pre-pass: a phi is resolved later by the propagation fixpoint, and a function parameter is rewritten byspecializeFuncfrom its actual argument's address space when callers are specialized, which runs after this pre-pass. Its declared surface pointee (int*→Device) is therefore only provisional. Reading it would wrongly reject valid code: a[noinline]helper whoseint*parameter is passed a descriptor-backed element pointer is specialized toStorageBuffer, so merging it with another element pointer is consistent —get-address-buffer-element-param-merge.slangpins that this compiles. So the pre-pass lets a slot's concrete writes drive reconciliation and leaves parameters to their own resolving mechanisms.The whole pre-pass (and its conflict diagnostic) is gated on the presence of a
DiagnosticSink, which only the SPIR-V legalizer supplies. The bug is SPIR-V-specific (logicalStorageBuffervs physicalPhysicalStorageBuffer); running the slot retyping on the Metal/WGSL/GLSL callers — which pass no sink — would silently change their slot types with no diagnostic on a genuine conflict and no test coverage, so those backends are intentionally left unchanged by this PR.Scope boundaries — pre-existing limitations, deliberately out of scope. Three related shapes are not addressed here; each is pre-existing, affects
&buf[i]identically, and is distinct from the address-space repair:[noinline]callee (and related result-only shapes) is not resolved. Making this general is the "substantial shared specialization work" the maintainer explicitly said should land as a separate prerequisite PR; it is tracked in Generalize pointer-slot address-space specialization: unify slot reconciliation with SSA/phi propagation and cross-function results (follow-up to #12592) #13039 (and relates to slangc never terminates on -target spirv for a function returning Optional<T*> #12498 / SPIR-V: Optional<T*> returning a function-local pointer emits invalid SPIR-V (function-local escape via Optional slot) #12562).Foo_naturalvsFoo_std430), not address space — because the surfaceint*/Foo*usesDefaultDataLayout(chosen in Part 1 to match&buf[i]) while the buffer element uses the buffer's layout. This never occurs for scalar pointees (no layout variance), which is why everyint*/scalar case here passes; fixing it means changingoperator&'s return-type semantics (the same separate work above).StorageBufferpointer ((float*)s) is rejected by SPIR-V as a logical-pointer operand; this is a logical-addressing limitation, not an address-space-propagation gap.&buf[i], and both requiring reconciliation after call specialization (tracked in Generalize pointer-slot address-space specialization: unify slot reconciliation with SSA/phi propagation and cross-function results (follow-up to #12592) #13039): (1) a slot that merges a physical-pointer parameter with aStorageBufferelement is not diagnosed here — a direct physical pointer (e.g. an integer-to-pointer cast, as inpointer-address-space-conflict.slang) still is; and (2) under-g, a pointer parameter specialized fromDevicetoStorageBufferleaves its debug backing variable with the old pointee. In both, because SPIR-V validation is opt-in (SLANG_RUN_SPIRV_VALIDATION=1), the current result can be ill-typed SPIR-V that is only caught when validation is enabled, rather than a guaranteed compile error. This PR does not widen that gap — it fixes the local-slot case (the actual__getAddresson a structured-buffer element (buf[i]) is rejected as a function-local address on all targets #12581 failure) and leaves the parameter/debug-var cases to the post-specialization work.What is covered and tested here is the default direct-SPIR-V
StorageBufferpath for scalar/consistent-layout pointees — direct, derived (s + 1), control-flow re-assignment, surviving slot,-O0/-g. One thing is handled by the code but not directly exercised by these tests: the pre-SPIR-V-1.4Uniformstructured-buffer storage class is not tested here simply because noUniform-specific test was added; the reconciliation itself is driven by the stored value's actual address space, so it is not hardcoded toStorageBufferand should handleUniformidentically. (TheresolvePhiAddrSpacejoin runs for the pass's pre-phi-elimination callers but is not the conflict-diagnosis path — see above.)Tests.
get-address-buffer-element-debug.slangruns the CI-failing shape at-O0(slot survives) and-g2 -O0(debug backing var materializes): the__getAddressand&spellings with a control-flow re-assignment and a derivedp[2], plus the derived-off-a-surviving-slot cascade (s/d, using&) and a control-flow-merged pointer stored into a further slot. Both runs assertCHECK-NOT: PhysicalStorageBuffer(withCHECKDBG-NOT:on the-g2run). Asserting the logicalStorageBufferpointer is merely present is not discriminating — the element value is always aStorageBufferpointer, so it appears in the buggy output too; the bug is that the slot's contained pointee stays physical (_ptr_Function__ptr_PhysicalStorageBuffer_int). This file takes no integer-to-pointer cast, so a correct compile emits no physical pointer at all, and the buggy slot type trips theCHECK-NOT. (Verified: with the reconcile pre-pass disabled, this test fails on exactly that slot type. slang-test removes-gembedded source for SPIR-V targets before FileCheck, so theCHECK-NOTdoes not self-match the source comment.) The conflict path is covered bypointer-address-space-conflict.slangat both optimization levels (a physical pointer from an integer cast, merged with aStorageBufferelement).get-address-buffer-element-param-merge.slangguards the opposite failure: a buffer-element pointer flowing through a[noinline]helper'sint*parameter and merged with another element pointer must not be falsely flagged as a conflict. It runs in non-debug builds (default and-O0), where the parameter is specialized toStorageBufferand the merge is consistent; it deliberately omits-g, which would hit the pre-existing specialized-parameter debug-var gap noted above (deferred to #13039).Part 1 tests (unchanged from the earlier revision)
The primary regression (
get-address-buffer-element.slang) exercises read/arithmetic/write on both-cpuand-vk.get-address-buffer-element-runtime.slangadds-cpu/-vkruntime coverage for a member-of-element address (read + write-back) and cross-element pointer arithmetic on a struct-element buffer, so a wrong field offset or element stride is executed, not just compiled. The non-default-layout stride question is answered statically bytests/spirv/get-address-buffer-element-layout.slang, which pinsArrayStride 16on aStd140struct-element pointer (identical to&buf[i]); a runtimeStd140test is not possible because an explicit-layout structured buffer is not runtime-executable in the harness even for a plain read (SPIR-V returns an empty readback; CPU/CUDA reject it with E36107) — a limitation of the buffer type, independent of__getAddress. The five added pointer tests cover deref forms, arithmetic,Ptr<T>/T*equivalence,ImmutablePtr, and subscript on-cpuand-vk, exceptpointer-immutable.slang, which is-cpu-only because converting toImmutablePtrand passing it to a function currently mis-lowers to SPIR-V (invalidOpBitcaston a logical pointer) for both this and&buf[i]— a pre-existing codegen limitation unrelated to this change. The two negative tests are flipped for the mutable case while keeping the read-onlyStructuredBufferrejection pinned.Known limitation from Part 1, deliberately out of scope. A custom-layout element address that escapes through a plain
T*(default-layout) function parameter can lose its non-default stride. This affects&buf[i]identically today (its declared return type is default-layoutPtr<T>) — it is a pre-existing property of both spellings, not introduced here. Fixing it would require changingoperator&'s declared return type incore.meta.slang, a semantics change for every existing&buf[i]user, which is well beyond lifting an E31160 rejection.Swizzle of a buffer element (investigated — no new risk, no silent miscompile). Lifting E31160 does not widen the addressable surface beyond
&. A single-component swizzle —__getAddress(buf[i].x)on aRWStructuredBuffer<float4>— is accepted and lowers to the byte-identical logicalStorageBufferOpAccessChainthat&buf[i].xproduces (verified for.xand.y: same access chain, same storage class). A single component is a genuine contiguous addressable sub-location, so this is correct. A multi-component or reordered swizzle (.xy,.yx,.xz) is rejected for both spellings —__getAddresswith E31160 and&with E40008 — so a non-contiguous or reordered swizzle can never silently become a pointer. The accepted swizzle surface is thus exactly&'s, consistent with the__getAddress(x) ≡ &xcontract.Closes #12581.
🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify.