Recover texture [format] across function parameters on CUDA (#12737) - #12766
Draft
nv-slang-bot[bot] wants to merge 1 commit into
Draft
Recover texture [format] across function parameters on CUDA (#12737)#12766nv-slang-bot[bot] wants to merge 1 commit into
nv-slang-bot[bot] wants to merge 1 commit into
Conversation
Specialize functions taking writable-texture parameters on CUDA so the concrete global carrying the [format] decoration reaches the surface-access site. Without this, a formatted RWTexture passed through a function parameter emitted the wrong surface byte stride (element size instead of backing-format size) and skipped the _convert read/write path, because _findImageFormatDecoration cannot recover the format from an undecorated IRParam. This is the same producer-side mechanism Khronos/WGSL already use, and covers reads, writes, and distinct-format call sites that pass concrete globals. Extract the writable-texture-access test into a shared isWritableTextureType helper and reuse it in isIllegalGLSLParameterType. Partial fix (does not auto-close #12737): array-of-globals elements and runtime-selected texture arguments still cannot recover the format and are left unspecialized (never worse than before); a diagnostic backstop is a coordinated follow-up with #12672. Addresses #12737.
Contributor
|
Automated notice (PR board sync) — do not reply to this comment. Auto-assigned @kaizhangNV as shepherd for this Bot PR. |
1 similar comment
Contributor
|
Automated notice (PR board sync) — do not reply to this comment. Auto-assigned @kaizhangNV as shepherd for this Bot PR. |
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
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.
Motivation
On the CUDA target, a formatted writable texture accessed through a function parameter emits a different (and wrong) surface access than the same texture accessed directly. The compiler fails to recover the
[format]backing, so it emits the plain, non-converting intrinsic and computes the coordinate scale from the access element size instead of the backing format. Consider this shader (the type used by the committed regression test):slangc repro.slang -target cuda -entry computeMain -stage computeemitted, before this change:CUDA surface access is byte-addressed on the x coordinate. A plain
surf2Dwrite<T>therefore scales the coordinate by the backing element size, so when the backing format differs from the access type the correct plain stride would be the 2-byter16fsize, not the 4-bytefloatsize — emitting* 4writes at the wrong texel and can corrupt adjacent memory. When a conversion is required (as here), the correct form is insteadsurf2Dwrite_convert<T>with an unscaled coordinate (* 1): the_converthelper handles the packing, so the emitter deliberately sets the stride multiplier to 1 for a converting write. Either way the through-parameter access is wrong because the format is not recovered — it emits the plain, unscaled-by-backing-size form with no_convert. No diagnostic is emitted.(The issue #12737 reports this with a
[format("r32ui")] RWTexture2D<uint64_t>example. That combination exposes the same format-recovery bug, but its converting intrinsicsurf2Dwrite_convert<ulonglong>has no CUDA-prelude implementation, so even a direct access to it fails NVRTC independently of this bug — a separate prelude gap. This PR therefore demonstrates and tests the fix withr16f/float, whose converting intrinsics the prelude does define, so the fix is verifiable end-to-end.) The same format-recovery limitation would affect thesuredatomic lowering proposed in #12672.Proposed solution
The
[format("...")]backing format is stored as anIRFormatDecorationon the global texture parameter. At the surface-access site the emitter recovers it with_findImageFormatDecoration(slang-intrinsic-expand.cpp), which reads the decoration off the resource instruction. When the texture is passed through a function, the access site sees only the callee'sIRParam— which carries no decoration — so_calcBackingElementSizeInBytesfalls back to the element-type size and the$Estride multiplier /$C_convertselection are computed from the wrong information.This is fundamentally the "smuggle a resource through a function" problem documented in the long
TODO(JS)/TimFcomment atslang-intrinsic-expand.cpp:191-214: from inside the callee you can only reach theIRParam, and distinct call sites may pass differently-formatted textures, so a single un-specialized callee cannot carry one correct format. That same comment notes the Vulkan back-end "gets away with this kind of coincidentally, since the legalization we have to do for resources means there wouldn't be a singlef()function any more."The principled, producer-side fix is to give CUDA that same property for the affected case: specialize functions that take a writable-texture parameter so the concrete global (carrying
[format]) is referenced directly at the access site. This is exactly the mechanism Khronos (SPIRV/GLSL) and WGSL already use — the resource-parameter specialization pass (specializeResourceUsage) already runs unconditionally for all targets (slang-emit.cpp); it is not gated behindshouldLegalizeExistentialAndResourceTypes. Only the per-parameter decisionResourceParameterSpecializationCondition::doesParamWantSpecializationreturnedfalsefor CUDA. Adding a CUDA arm to that decision fixes surface read/write — and the pendingsuredatomic path, which shares_calcBackingElementSizeInBytes— uniformly, with no change to the emitter.Once the callee refers to the global directly,
_findImageFormatDecorationfinds the decoration,_calcBackingElementSizeInBytesreturns the true backing size, and the through-parameter access emits identically to the direct access.Scope. This fixes the format-recovery defect the issue reports — a formatted writable texture passed through a function parameter where the argument is a concrete global — for both reads and writes wherever the target's converting helper exists, and handles distinct-format call sites via distinct specializations. It intentionally does not auto-close #12737: four residual cases still emit a possibly-wrong stride (all of them no worse than the base compiler today — see the Process report for the verification), and #12737 stays open to track them:
f(gTexArray[i])) — which is a separate, pre-existing format-recovery gap, not the parameter-boundary bug (it reproduces with a direct access and no function parameter at all);getTex()[coord]) — not foldable to a single global;f(cond ? gA : gB)) — inherently unrecoverable, since the two textures may carry different formats;canSpecializeCallis all-or-nothing per call, so it leaves the foldable one unspecialized too.Closing these needs a separate format-recovery fix for array/returned resources and/or a capability diagnostic (the E41405-style approach being added in #12672); that is deliberately left as a coordinated follow-up rather than raced against that PR. The predicate is scoped to writable textures for consistency with the existing Khronos/WGSL arm; specializing an unformatted writable texture is correctness-neutral (its stride is unchanged) at a bounded code-size cost.
Change summary
source/slang/slang-ir-specialize-resources.cppdoesParamWantSpecializationthat requests specialization for writable-texture parameters. Extract the "writable texture access" test into a sharedisWritableTextureTypehelper and reuse it in the existingisIllegalGLSLParameterTypeso the classification lives in one place.tests/compute/texture-format-through-param.slangSIMPLE(filecheck=CUDA,READ,NEG)regression test: a[format("r16f")] RWTexture2D<float>written and read through a function parameter (and written directly) must now emit the converting intrinsics (surf2Dwrite_convertwith the unscaled* 1coordinate;surf2Dread_convertwith the 2-byte backing scale), matching the direct access, with no plain non-converting form. Compiles end-to-end via NVRTC (-target ptx).Concepts and vocabulary
doesParamWantSpecialization(ResourceParameterSpecializationCondition) — the per-parameter predicate that decides whether a function taking this parameter should be specialized per concrete argument. It already returnstrueforConstantBuffer<T>on all targets and for illegal buffer/image/texture parameter types on Khronos/WGSL; every other target previously fell through tofalse.isParamSuitableForSpecialization/canSpecializeCall(slang-ir-specialize-function-call.cpp) — the companion predicate that decides whether a given argument can actually be folded. It accepts several shapes (global shader parameters, global values, user-pointer types, descriptor-handle casts, and index/load/field chains rooted at one of those); for the texture-resource path relevant to this PR that means the argument must resolve to a global (or an index into a global array). If a parameter wants specialization but the argument is not suitable,canSpecializeCallreturnsfalseand the whole call is left unspecialized — this all-or-nothing behavior is why the "mixed call" residual below exists.$E/$C(slang-intrinsic-expand.cpp) — the surface-intrinsic template expansions:$Cappends_convertto the intrinsic name when a format conversion is required;$Eemits the coordinate scale factor — the backing-element byte size for a plain (non-converting) access, but1for a converting write (_calcBackingElementSizeInBytesis overridden to 1 there, since the_converthelper handles packing). Both read the recoveredIRFormatDecoration.Process report
The one behavioral change — a CUDA arm in
doesParamWantSpecialization. The bug's root cause is that the format decoration is unreachable across the parameter boundary; the fix makes it reachable by specializing the callee against the concrete global, rather than patching the emitter to guess a stride it cannot prove. Requesting specialization is scoped toisWritableTextureType(type)—RWTexture/WTexture/rasterizer-ordered views — because only writable-texture accesses go through the CUDAsurf*write/surf*readsize-scaling path; a sampledTexture(plainREAD) lowers to a texture fetch and does not need it. It is gated byisCUDATarget(targetRequest)(matching the siblingisKhronosTarget/isWGPUTargetarms). CUDA is the only C-family target that reaches this path:CUDASourceEmitterinheritsCPPSourceEmitter, but the surface intrinsics (surf2Dwrite$C…$E) are CUDA-only templates inhlsl.meta.slang, and the CPU/C++ path never emits them.Input-shape check (is this the right layer?). Yes. The
IRParamreaching the access site is not a malformed shape to be repaired at emit; it is the correct IR for an un-specialized callee. The problem is that CUDA left the callee un-specialized where the format information only survives on the global. The producer-side fix — specialize so the global reaches the access site — is precisely how the other targets avoid the same bug, and it removes the need for any emit-time guess. The alternative of carrying the format on theIRTextureTypeand reading it at emit was rejected: it founders on the multi-call-site ambiguity in theTODO(one un-specialized callee cannot carry one correct format when call sites disagree), and it would introduce a second format representation (decoration vs. type operand) that every consumer would have to agree on, violating "one canonical representation per value."What is fixed, and what is not (verified against the base compiler). The fix recovers the format when the argument is a concrete global texture —
f(gTex)— for both reads and writes, and when different call sites pass differently-formatted globals (each gets its own specialization). Four cases are not covered and remain exactly as they emit today (never worse — verified against base):f(gTexArray[i])): this is a separate, pre-existing bug, not the parameter-boundary bug. It reproduces with a direct access and no function parameter at all —gTexArray[idx][coord] = valready emitssurf2Dwrite<...> .x * 4on the base compiler (verified identical on this branch). The call here does specialize (the suitability check tracesGetElementto the global), but_findImageFormatDecorationdoes not recover the per-array[format]off the array-element access; fixing that lives in_findImageFormatDecoration(the same file Lower RWTexture atomics to PTX sured on CUDA/PTX (#12636) #12672 is editing), independent of this change.getTex()[coord]): the argument is a call result, not foldable to a single global, so the callee is not specialized and the access emitssurf2Dwrite<...> .x * 4(verified).f(cond ? gA : gB)):isParamSuitableForSpecializationreturnsfalsefor the resource path here, socanSpecializeCallbails for the whole call. This one is inherently unrecoverable by specialization — the two textures may carry different formats.f(gTex, cond ? gA : gB)):canSpecializeCallis all-or-nothing per call, so one unfoldable argument leaves the foldablegTexunspecialized too.Closing these requires either recovering the format after array/GEP reconstruction, argument-level (vs call-level) specialization, or a capability diagnostic (the E41405-style approach being added in #12672). That is deliberately left as a coordinated follow-up rather than raced against #12672, which is why this PR does not auto-close #12737. The common concrete-global case the issue reports is fixed.
The refactor —
isWritableTextureType. The switch onIRTextureType::getAccess()for the writable access modes previously lived inline inisIllegalGLSLParameterType. Extracting it into a named helper and reusing it in both the GLSL-legality check and the new CUDA arm keeps the "which texture accesses are writable" classification in one place, so the two callers cannot drift. No behavior change to the GLSL path.Tests
tests/compute/texture-format-through-param.slang— a[format("r16f")] RWTexture2D<float>written through a function parameter and directly, and read through a parameter. It uses one multi-prefix source-emission FileCheck directive (SIMPLE -target cuda, which runsslangc+ FileCheck):CUDAasserts twosurf2Dwrite_convertcalls with stride* 1(the through-parameter write matches the direct one);READassertssurf2Dread_convert … .x * 2(the through-parameter read recovers the 2-byte backing size); and aNEGprefix forbids the plainsurf2Dwrite</surf2Dread<across the whole input. Fails before the fix (only the direct write converted; a plain.x * 4write and a plain read remained), passes after.r16f/floatis used (not an integer format) both to force the size-mismatch conversion and because the CUDA prelude only defines the converting-read helper (surf2Dread_convert) for float element types.-target ptx) rc=0 on an L40S with CUDA 12.6, confirming the specialization produces valid CUDA (not just textually-correct source) and does not trip an assert cloning an un-legalized texture global. (floatreads use the definedsurf2Dread_convert<float>overload; an integer converting read would hit a pre-existing prelude gap where only float overloads are defined — orthogonal to this fix.)tests/compute/{texture-subscript,rw-texture-simple,half-rw-texture-convert*},tests/hlsl-intrinsic/texture/, andtests/diagnostics/(GPU-only legs ignored); the GLSL predicate is behavior-identical after the refactor.Addresses #12737 (fixes the common concrete-global case for reads and writes; the runtime-selected-argument residual and a follow-up diagnostic are noted above — this PR intentionally does not auto-close the issue).