Add data layout parameter to pointers#10058
Open
juliusikkala wants to merge 14 commits intoshader-slang:masterfrom
Open
Add data layout parameter to pointers#10058juliusikkala wants to merge 14 commits intoshader-slang:masterfrom
juliusikkala wants to merge 14 commits intoshader-slang:masterfrom
Conversation
📝 WalkthroughWalkthroughPointer types gain an explicit data-layout parameter that is threaded through language-level pointer aliases, AST/IR pointer construction, legalization, emission, and buffer-lowering code; new buffer layout kinds, API adjustments, and tests validate layout-specific pointer/stride behaviour. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
|
This PR modifies IR instruction definition files. Please review if you need to update the following constants in
These version numbers help ensure compatibility between different versions of compiled modules. |
juliusikkala
commented
Feb 18, 2026
Contributor
Author
juliusikkala
left a comment
juliusikkala
left a comment
There was a problem hiding this comment.
Self-review for pointing out parts I don't personally like but don't know better ways.
|
|
||
| /// Represents a pointer with custom data layout as a shorthand for Ptr<T>. | ||
| /// Such pointers are almost always in the Device address space. | ||
| typealias LayoutPtr<T, L:IBufferDataLayout> = Ptr<T, Access.ReadWrite, AddressSpace.Device, L>; |
Contributor
Author
There was a problem hiding this comment.
This differs from the proposal as it is now, will update if there's no opposition to adding this typealias.
Comment on lines
+568
to
+578
| for (auto typeConstraintDeclRef : | ||
| containerDeclRef.getDecl()->getDirectMemberDeclsOfType<TypeConstraintDecl>()) | ||
| { | ||
| if (typeConstraintDeclRef->getSup().type == dataLayoutInterface) | ||
| { | ||
| subtypeWitness = getDeclaredSubtypeWitness( | ||
| dataLayoutType, | ||
| m_sharedASTBuilder->getIBufferDataLayoutType(), | ||
| typeConstraintDeclRef); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Because the data layout parameter has a constraint (IBufferDataLayout), we unfortunately have to dig the subtype witness here :/
| defaultPointerRules = IRTypeLayoutRules::get(IRTypeLayoutRuleName::D3DConstantBuffer); | ||
| else | ||
| defaultPointerRules = IRTypeLayoutRules::get(IRTypeLayoutRuleName::LLVM); | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This logic is now instead handled via getTypeLayoutRuleNameForBuffer, which now also operates with pointer types.
Comment on lines
-1558
to
-1592
| // Tries to find which layout rules apply to the given pointer, based on | ||
| // "provenance": we track the pointer to where we got it and check if the | ||
| // source is a buffer with a specific layout. | ||
| IRTypeLayoutRules* getPtrLayoutRules(IRInst* ptr) | ||
| { | ||
| // Check if the pointer is actually based on an buffer with an explicit | ||
| // layout. If so, we need to take that layout into account. | ||
| if (auto structuredBufferInst = as<IRRWStructuredBufferGetElementPtr>(ptr)) | ||
| { | ||
| auto baseType = cast<IRHLSLStructuredBufferTypeBase>( | ||
| structuredBufferInst->getBase()->getDataType()); | ||
| return getBufferLayoutRules(baseType); | ||
| } | ||
| else if (auto cbufType = as<IRConstantBufferType>(ptr->getDataType())) | ||
| { | ||
| return getBufferLayoutRules(cbufType); | ||
| } | ||
| else if (auto gep = as<IRGetElementPtr>(ptr)) | ||
| { | ||
| // Transitive | ||
| return getPtrLayoutRules(gep->getBase()); | ||
| } | ||
| else if (auto off = as<IRGetOffsetPtr>(ptr)) | ||
| { | ||
| // Transitive | ||
| return getPtrLayoutRules(off->getBase()); | ||
| } | ||
| else if (auto fieldAddr = as<IRFieldAddress>(ptr)) | ||
| { | ||
| // Transitive | ||
| return getPtrLayoutRules(fieldAddr->getBase()); | ||
| } | ||
| return defaultPointerRules; | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Good riddance, tbh. Really didn't like this code.
Comment on lines
+3078
to
+3080
| defaultPointerRules = getTypeLayoutRuleForBuffer( | ||
| codeGenContext->getTargetProgram(), | ||
| irBuilder.getPtrType(irBuilder.getVoidType())); |
Contributor
Author
There was a problem hiding this comment.
This is not super pretty, but I really want to use the common logic from getTypeLayoutRuleForBuffer everywhere so as not to duplicate it in each target.
Comment on lines
729
to
732
| ["Type.D3DConstantBufferLayout"] = 727, | ||
| ["Type.MetalParameterBlockLayout"] = 728, | ||
| ["Type.CUDALayout"] = 729, | ||
| ["Type.LLVMLayout"] = 730, |
Contributor
Author
There was a problem hiding this comment.
These IR insts are added because after this PR, RWStructuredBufferGetElementPtr and similar functions actually have an IR pointer type with the correct layout rule as an operand. So every layout rule needs to have a corresponding IR type now, not only the ones that are available to users.
Comment on lines
+2168
to
+2170
| auto dataLayout = isCPUTargetViaLLVM(target->getTargetReq()) | ||
| ? (IRType*)builder.getLLVMBufferLayoutType() | ||
| : (IRType*)builder.getCBufferLayoutType(); |
Contributor
Author
There was a problem hiding this comment.
This is actually a bit of a drive-by fix; the LLVM target is constructing ComputeThreadVaryingInput with contained vectors aligned to 16 bytes while the C++ target aligns to 4 bytes, so this just documents that correctly to the pointer type.
Comment on lines
+1649
to
+1651
| // Fix the pointer types of `RWStructuredBufferGetElementPtr` and | ||
| // `FieldAddress` to be layout-aware. | ||
| fixBufferAccessPointerTypes(module->getModuleInst()); |
Contributor
Author
There was a problem hiding this comment.
I don't like this, as it's fixing IR after-the-fact. But there's no clean alternative: the offending instructions RWStructuredBufferGetElementPtr, FieldAddress and GetElementPtr are emitted before the IR is specialized to any target, and as such, we cannot know which concrete layout type is being used by a buffer until running target-specific passes. Some parts of that layout selection is target-dependent and DefaultDataLayout means different things depending on buffer type.
Comment on lines
+2382
to
+2391
| if (auto ptrType = as<IRPtrTypeBase>(bufferType)) | ||
| { | ||
| auto layoutTypeOp = ptrType->getDataLayout() ? ptrType->getDataLayout()->getOp() | ||
| : kIROp_DefaultBufferLayoutType; | ||
|
|
||
| IRTypeLayoutRuleName defaultRule = IRTypeLayoutRuleName::Natural; | ||
| if (isCPUTargetViaLLVM(targetReq)) | ||
| defaultRule = IRTypeLayoutRuleName::LLVM; | ||
|
|
||
| return getTypeLayoutRulesFromOp(layoutTypeOp, defaultRule); |
Contributor
Author
There was a problem hiding this comment.
This function also now handles the layout of pointer types, as they're very similar.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
source/slang/slang-emit-llvm.cpp (1)
1707-1725:⚠️ Potential issue | 🟡 MinorIRTypeLayoutRules::get() can return null in the default case; verify all enum values are handled.
The code correctly handles pointer types via
getTypeLayoutRuleNameForBuffer, which explicitly checksIRPtrTypeBaseat line 2382 and respects thedataLayoutoperand with fallback tokIROp_DefaultBufferLayoutType. For the specific initialization ofdefaultPointerRulesviaPtr<void>, the pointer path returnsNatural, which is never null.However,
IRTypeLayoutRules::get()(line 907-930) has a default case returningnullptr. While all current code paths should avoid this (through proper enum handling), this design creates a latent vulnerability: if a newIRTypeLayoutRuleNameenum value is added without updating the switch statement, or ifgetTypeLayoutRulesFromOpmaps to an unmapped enum value,defaultPointerRulescould become null without detection. Consider asserting that the result is non-null, or ensuring all enum variants are exhaustively handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-emit-llvm.cpp` around lines 1707 - 1725, The call to IRTypeLayoutRules::get() can return nullptr in its default switch branch; update the code paths that assume a non-null result (notably the initialization of defaultPointerRules and uses via getTypeLayoutRuleNameForBuffer/getTypeLayoutRulesFromOp) to validate the returned pointer immediately—either assert/abort on nullptr or throw a clear error—so any unmapped IRTypeLayoutRuleName enum value is caught early; specifically, after calling IRTypeLayoutRules::get(...) check the result and fail fast with a descriptive message referencing the rule name (or add exhaustive handling in IRTypeLayoutRules::get to never return nullptr).source/slang/slang-ast-builder.cpp (1)
556-583:⚠️ Potential issue | 🟠 MajorGuard against invalid/null
dataLayoutTypeto prevent null pointer dereference crashes.The cast chain
as<DeclRefType>(dataLayoutType)->getDeclRef()dereferences immediately without validating the cast succeeded. IfdataLayoutTypeis null or fails the type check, this will crash. Error recovery paths during compilation may produce invalid types; a defensive fallback using the existinggetDefaultLayoutType()method ensures robustness:Proposed hardening
PtrTypeBase* ASTBuilder::getPtrType( Type* valueType, Val* accessQualifier, Val* addrSpace, Type* dataLayoutType, char const* ptrTypeName) { - auto declRef = as<DeclRefType>(dataLayoutType)->getDeclRef(); - auto containerDeclRef = declRef.as<ContainerDecl>(); + if (!dataLayoutType || !as<DeclRefType>(dataLayoutType)) + dataLayoutType = getDefaultLayoutType(); + auto declRefType = as<DeclRefType>(dataLayoutType); + SLANG_RELEASE_ASSERT(declRefType); + auto declRef = declRefType->getDeclRef(); + auto containerDeclRef = declRef.as<ContainerDecl>(); + SLANG_RELEASE_ASSERT(containerDeclRef.getDecl()); auto dataLayoutInterface = m_sharedASTBuilder->getIBufferDataLayoutType(); DeclaredSubtypeWitness* subtypeWitness = nullptr; - for (auto typeConstraintDeclRef : - containerDeclRef.getDecl()->getDirectMemberDeclsOfType<TypeConstraintDecl>()) + auto containerDecl = containerDeclRef.getDecl(); + for (auto typeConstraintDeclRef : + containerDecl->getDirectMemberDeclsOfType<TypeConstraintDecl>()) { if (typeConstraintDeclRef->getSup().type == dataLayoutInterface) { subtypeWitness = getDeclaredSubtypeWitness( dataLayoutType, m_sharedASTBuilder->getIBufferDataLayoutType(), typeConstraintDeclRef); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-ast-builder.cpp` around lines 556 - 583, In ASTBuilder::getPtrType, defensively validate dataLayoutType before doing as<DeclRefType>(dataLayoutType)->getDeclRef(): if dataLayoutType is null or not a DeclRefType, replace it with m_sharedASTBuilder->getDefaultLayoutType() and then proceed; use the (possibly replaced) dataLayoutType when obtaining declRef, containerDeclRef, and when calling getDeclaredSubtypeWitness(..., m_sharedASTBuilder->getIBufferDataLayoutType(), ...); preserve the existing SLANG_RELEASE_ASSERT(subtypeWitness) and pass the resulting subtypeWitness into getSpecializedBuiltinType as before so you avoid null-dereference crashes while keeping current behavior.source/slang/slang-ir-legalize-varying-params.cpp (1)
2158-2175:⚠️ Potential issue | 🟠 MajorPropagate dataLayout to field pointer types for layout consistency.
uint3PtrTypeis created before the dataLayout decision (line 2160), butvaryingInputStructPtrTypeis created with target-specific layout (lines 2168–2175). WhenemitFieldAddress(uint3PtrType, varyingInputParam, ...)is called on lines 2226 and 2229, the field pointers lack the struct's layout information, breaking address/offset calculations under LLVM vs C layouts.Proposed fix
- uint3PtrType = builder.getPtrType(uint3Type); - // As we construct the `ComputeThreadVaryingInput` type and its fields, // we mark them all as target intrinsics, which means that their // declarations will *not* be reproduced in the output code, instead // coming from the "prelude" file that already defines this type. varyingInputStructType = builder.createStructType(); auto dataLayout = isCPUTargetViaLLVM(target->getTargetReq()) ? (IRType*)builder.getLLVMBufferLayoutType() : (IRType*)builder.getCBufferLayoutType(); + uint3PtrType = builder.getPtrType( + uint3Type, + AccessQualifier::Read, + AddressSpace::Generic, + dataLayout); varyingInputStructPtrType = builder.getPtrType( varyingInputStructType, AccessQualifier::Read, AddressSpace::Generic, dataLayout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-ir-legalize-varying-params.cpp` around lines 2158 - 2175, The uint3PtrType is created before dataLayout is chosen so field pointers lack the struct's layout and produce incorrect addresses when emitFieldAddress(uint3PtrType, ...) is used; fix by creating the uint3PtrType (via builder.getPtrType on the uint3 vector type) using the same dataLayout as varyingInputStructPtrType (i.e., move or recreate the uint3PtrType construction after dataLayout is computed or rebuild it with dataLayout) so both the struct pointer type and the field pointer types share the same layout before calls to emitFieldAddress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/core.meta.slang`:
- Around line 1364-1366: The constructor __init<U, Access accessOther,
L:IBufferDataLayout>(Ptr<U, accessOther, addrSpace, L> ptr) shadows the
struct-level generic L which permits bit-casting between pointers with different
data layouts and can produce incorrect offsets at dereference; update the code
by either renaming the constructor generic (e.g., LOther) to avoid shadowing or
add a short comment beside __init/Ptr clarifying that callers are responsible
for ensuring layout compatibility when converting between layouts (mentioning
Std140 vs Scalar as an example) so the potential layout-mismatch risk is
explicit.
In `@source/slang/slang-emit-spirv.cpp`:
- Around line 7738-7746: The code currently only sets dataLayout from
destPtrType when destPtrType->hasAddressSpace() is true, causing implicit
(Generic) address-space pointers with an explicit layout to be ignored; change
the logic in the block that builds ptrElementType so that address space still
comes from destPtrType only if hasAddressSpace() is true, but always attempt to
read dataLayout from destPtrType (e.g. call destPtrType->getDataLayout() and use
it if non-null/valid); update the code around AddressSpace::Function,
builder.getDefaultBufferLayoutType(), destPtrType->hasAddressSpace(),
destPtrType->getDataLayout(), and builder.getPtrType(...) to propagate layout
independently of address-space presence.
In `@source/slang/slang-ir-glsl-legalize.cpp`:
- Around line 1760-1765: The derived field pointer types in extractField and
getSubscriptVal are losing pointer attributes because they call
getPtrType(ptrType->getOp(), fieldType) / getPtrType(elementType) without
propagating the parent pointer's access qualifier, address space, and data
layout; update both sites to use getPtrTypeWithAddressSpace(fieldType, ptrType)
(or otherwise call getPtrType with ptrType->getAccessQualifier(),
ptrType->getAddressSpace(), ptrType->getDataLayout()) so the new pointer
preserves ptrType's attributes and maintains IR type invariants.
In `@source/slang/slang-ir-lower-buffer-element-type.cpp`:
- Around line 2057-2065: getPointerTypeWithBufferLayout currently hardcodes
AccessQualifier::ReadWrite and AddressSpace::Generic; change it to mirror
copyBufferLayoutToPointer by extracting the pointer qualifiers from bufferType
(cast to IRPtrType or use its accessor) and passing those AccessQualifier and
AddressSpace values into builder.getPtrType alongside elementType and layoutType
(use getBufferTypeLayoutType(builder, bufferType) for layout). Ensure you handle
the case where bufferType is not a pointer consistently with
copyBufferLayoutToPointer.
In `@source/slang/slang-ir-spirv-legalize.cpp`:
- Around line 238-243: The synthesized pointer created via
builder.getPtrType(...) inside builder.emitFieldAddress currently uses
builder.getDefaultBufferLayoutType(), which discards any explicit dataLayout
from the original buffer; change the code that constructs the new pointer (the
call to builder.getPtrType in the emitFieldAddress site and the similar sites at
the other locations) to retrieve and pass the originating pointer's dataLayout
(e.g. use oldPtrType->getDataLayout() or an extracted dataLayout value when
available) instead of getDefaultBufferLayoutType(), so the new ptrType preserves
the original buffer layout and ptrType->getDataLayout() remains non-null for
downstream passes (see usages of oldPtrType->getDataLayout() and
ptrType->getDataLayout() elsewhere).
In `@tests/language-feature/pointer/data-layouts.slang`:
- Around line 1-2: Replace the GPU-backed compute directives that use
`-llvm`/`-vk` and `-compute` (the lines starting with
`//TEST(compute):COMPARE_COMPUTE_EX(...)`) with CPU/interpreter compute
directives so the test is GPU-free; specifically, change those directives to use
the CPU compute driver and output-using-type form (for example use the pattern
`//TEST:COMPARE_COMPUTE(filecheck-buffer=CHECK):-cpu -output-using-type`) so the
test runs on the CPU/interpreter rather than LLVM/VK GPU paths.
---
Outside diff comments:
In `@source/slang/slang-ast-builder.cpp`:
- Around line 556-583: In ASTBuilder::getPtrType, defensively validate
dataLayoutType before doing as<DeclRefType>(dataLayoutType)->getDeclRef(): if
dataLayoutType is null or not a DeclRefType, replace it with
m_sharedASTBuilder->getDefaultLayoutType() and then proceed; use the (possibly
replaced) dataLayoutType when obtaining declRef, containerDeclRef, and when
calling getDeclaredSubtypeWitness(...,
m_sharedASTBuilder->getIBufferDataLayoutType(), ...); preserve the existing
SLANG_RELEASE_ASSERT(subtypeWitness) and pass the resulting subtypeWitness into
getSpecializedBuiltinType as before so you avoid null-dereference crashes while
keeping current behavior.
In `@source/slang/slang-emit-llvm.cpp`:
- Around line 1707-1725: The call to IRTypeLayoutRules::get() can return nullptr
in its default switch branch; update the code paths that assume a non-null
result (notably the initialization of defaultPointerRules and uses via
getTypeLayoutRuleNameForBuffer/getTypeLayoutRulesFromOp) to validate the
returned pointer immediately—either assert/abort on nullptr or throw a clear
error—so any unmapped IRTypeLayoutRuleName enum value is caught early;
specifically, after calling IRTypeLayoutRules::get(...) check the result and
fail fast with a descriptive message referencing the rule name (or add
exhaustive handling in IRTypeLayoutRules::get to never return nullptr).
In `@source/slang/slang-ir-legalize-varying-params.cpp`:
- Around line 2158-2175: The uint3PtrType is created before dataLayout is chosen
so field pointers lack the struct's layout and produce incorrect addresses when
emitFieldAddress(uint3PtrType, ...) is used; fix by creating the uint3PtrType
(via builder.getPtrType on the uint3 vector type) using the same dataLayout as
varyingInputStructPtrType (i.e., move or recreate the uint3PtrType construction
after dataLayout is computed or rebuild it with dataLayout) so both the struct
pointer type and the field pointer types share the same layout before calls to
emitFieldAddress.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/slang/slang-ir-spirv-legalize.cpp (1)
447-532:⚠️ Potential issue | 🟠 MajorApply buffer-specific layout derivation instead of relying on generic default fallback.
When
getDataLayout()returns null for uniform parameter groups, structured buffers, or GLSL shader storage buffers,getPtrType()falls back togetDefaultBufferLayoutType(). However, the correct layout should be derived per-buffer-type usinggetTypeLayoutTypeForBuffer(), which accounts for target-specific layout rules. This ensures proper data packing, matching the pattern already used at line 243 for constant buffer parameters.🔧 Fix: Use derived layout instead of generic fallback
- dataLayout = uniformParamGroupType->getDataLayout(); + if (auto layout = uniformParamGroupType->getDataLayout()) + dataLayout = layout; + else + dataLayout = getTypeLayoutTypeForBuffer( + m_sharedContext->m_targetProgram, + builder, + uniformParamGroupType); ... - dataLayout = structuredBufferType->getDataLayout(); + if (auto layout = structuredBufferType->getDataLayout()) + dataLayout = layout; + else + dataLayout = getTypeLayoutTypeForBuffer( + m_sharedContext->m_targetProgram, + builder, + structuredBufferType); ... - dataLayout = glslShaderStorageBufferType->getDataLayout(); + if (auto layout = glslShaderStorageBufferType->getDataLayout()) + dataLayout = layout; + else + dataLayout = getTypeLayoutTypeForBuffer( + m_sharedContext->m_targetProgram, + builder, + glslShaderStorageBufferType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-ir-spirv-legalize.cpp` around lines 447 - 532, The code currently uses builder.getDefaultBufferLayoutType() as a fallback when innerType->getDataLayout() is null (e.g., in the IRUniformParameterGroupType, IRHLSLStructuredBufferTypeBase/derived, and IRGLSLShaderStorageBufferType branches), which is wrong; instead call the buffer-specific layout derivation helper (getTypeLayoutTypeForBuffer) to compute the correct layout. Update the branches that set dataLayout (where code currently does dataLayout = uniformParamGroupType->getDataLayout(), dataLayout = structuredBufferType->getDataLayout(), and dataLayout = glslShaderStorageBufferType->getDataLayout()) so that if getDataLayout() returns null you call getTypeLayoutTypeForBuffer(innerType) (or the corresponding builder helper used in this codebase) and assign that to dataLayout before any further use (this will ensure getPtrType()/pointer creation uses the proper buffer-specific layout rather than getDefaultBufferLayoutType()). Ensure you reference IRUniformParameterGroupType, lowerStructuredBufferType/IRHLSLStructuredBufferTypeBase, and IRGLSLShaderStorageBufferType when making the replacement so the change is applied in each of those branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-ir-lower-buffer-element-type.cpp`:
- Around line 2054-2059: The formatting of the block that calls
getTypeLoweringConfigForBuffer, getTypeLayoutTypeForBuffer and
builder.getPtrType does not conform to the project's clang-format rules; re-run
the project's formatter (clang-format v17-18) or run ./extras/formatting.sh on
source/slang/slang-ir-lower-buffer-element-type.cpp to apply the required C++
formatting and commit the updated file so CI's clang-format check passes.
---
Outside diff comments:
In `@source/slang/slang-ir-spirv-legalize.cpp`:
- Around line 447-532: The code currently uses
builder.getDefaultBufferLayoutType() as a fallback when
innerType->getDataLayout() is null (e.g., in the IRUniformParameterGroupType,
IRHLSLStructuredBufferTypeBase/derived, and IRGLSLShaderStorageBufferType
branches), which is wrong; instead call the buffer-specific layout derivation
helper (getTypeLayoutTypeForBuffer) to compute the correct layout. Update the
branches that set dataLayout (where code currently does dataLayout =
uniformParamGroupType->getDataLayout(), dataLayout =
structuredBufferType->getDataLayout(), and dataLayout =
glslShaderStorageBufferType->getDataLayout()) so that if getDataLayout() returns
null you call getTypeLayoutTypeForBuffer(innerType) (or the corresponding
builder helper used in this codebase) and assign that to dataLayout before any
further use (this will ensure getPtrType()/pointer creation uses the proper
buffer-specific layout rather than getDefaultBufferLayoutType()). Ensure you
reference IRUniformParameterGroupType,
lowerStructuredBufferType/IRHLSLStructuredBufferTypeBase, and
IRGLSLShaderStorageBufferType when making the replacement so the change is
applied in each of those branches.
---
Duplicate comments:
In `@source/slang/slang-ir-glsl-legalize.cpp`:
- Around line 2597-2599: getSubscriptVal currently builds the element pointer
with builder->getPtrType(elementType) which loses pointer attributes; update the
ScalarizedVal::address branch in getSubscriptVal so it derives the element
pointer type from the parent pointer type of val.irValue (propagating access
qualifier, address space and data layout) and then call
builder->emitElementAddress with that propagated pointer type (use the existing
val.irValue pointer type's attribute accessors instead of the default getPtrType
overload).
In `@source/slang/slang-ir-lower-buffer-element-type.cpp`:
- Around line 2050-2059: getPointerTypeWithBufferLayout currently hardcodes
AccessQualifier::ReadWrite and uses loweringConfig.addressSpace directly which
can widen or drop qualifiers when bufferType is already a pointer; change it to
mirror copyBufferLayoutToPointer by extracting the pointer qualifiers and
address space from bufferType (when bufferType is a pointer) and pass those into
builder.getPtrType instead of unconditionally using ReadWrite/generic; use
getTypeLoweringConfigForBuffer and getTypeLayoutTypeForBuffer as needed, but
preserve bufferType's existing access qualifier and addressSpace when creating
the new pointer for elementType so qualifiers are not widened or lost.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/slang/slang-ir-lower-buffer-element-type.cpp`:
- Around line 2050-2062: getPointerTypeWithBufferLayout currently hardcodes
AccessQualifier::ReadWrite, which can widen read-only buffers; change it to
derive the qualifier from the incoming bufferType (or its pointer
representation) instead of using AccessQualifier::ReadWrite. In the body of
getPointerTypeWithBufferLayout, obtain the correct access qualifier by querying
bufferType (or its pointer type) via the existing helper/API used elsewhere
(e.g. getAccessQualifier or the bufferType's pointer-like accessor) and pass
that qualifier into builder.getPtrType along with the existing
loweringConfig.addressSpace and layoutType (keeping
getTypeLoweringConfigForBuffer and getTypeLayoutTypeForBuffer calls intact).
Ensure you handle both raw pointer bufferType and structured-buffer wrapper
types the same way so read-only qualifiers are preserved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/slang/slang-ir-insts.lua (1)
239-283:⚠️ Potential issue | 🟠 MajorUpdate
k_maxSupportedModuleVersioninsource/slang/slang-ir.hfor the new IR schema changes.The file header in
slang-ir-insts.luaexplicitly states to update module versions when modifying this file. Adding thedataLayoutoperand toPtr,RefParam,BorrowInParam, andPseudoPtrtypes (lines 245, 256, 267, 282) plus the four new layout types (lines 343-346) constitutes a schema change requiring version bump. Per the documentation inslang-ir.h(lines 2128-2130), onlyk_maxSupportedModuleVersionneeds to be incremented for new instructions/operands;k_minSupportedModuleVersionremains unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-ir-insts.lua` around lines 239 - 283, You added a schema-changing IR operand (the "dataLayout" operand) to Ptr, RefParam, BorrowInParam, and PseudoPtr and introduced four new layout types in slang-ir-insts.lua, so increment the module version constant: open source/slang/slang-ir.h and bump k_maxSupportedModuleVersion (leave k_minSupportedModuleVersion unchanged) to the next integer value so the new instructions/operands are recognized as a newer supported module version.
🧹 Nitpick comments (1)
source/slang/slang-ir-insts.h (1)
3353-3363: Update comment to reflect that data layout is also copied.The comment lists the properties being copied but omits the newly-added
dataLayout. Consider updating for accuracy.📝 Suggested comment update
- // Copies the op-type of the oldPtrType, access-qualifier and address-space. + // Copies the op-type of the oldPtrType, access-qualifier, address-space, and data-layout. // Does not reuse the same `inst` for access-qualifier and address-space.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-ir-insts.h` around lines 3353 - 3363, The comment above getPtrType(IRType*, IRPtrTypeBase*) incorrectly omits that data layout is also copied from oldPtrType; update the comment to list that the op-type, access-qualifier, address-space, and dataLayout are copied (and clarify the note about not reusing the same `inst` for access-qualifier and address-space) so it accurately reflects the parameters passed to the overloaded getPtrType call using oldPtrType->getOp(), getAccessQualifier(), getAddressSpace(), and getDataLayout().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@source/slang/slang-ir-insts.lua`:
- Around line 239-283: You added a schema-changing IR operand (the "dataLayout"
operand) to Ptr, RefParam, BorrowInParam, and PseudoPtr and introduced four new
layout types in slang-ir-insts.lua, so increment the module version constant:
open source/slang/slang-ir.h and bump k_maxSupportedModuleVersion (leave
k_minSupportedModuleVersion unchanged) to the next integer value so the new
instructions/operands are recognized as a newer supported module version.
---
Duplicate comments:
In `@source/slang/slang-ir-lower-buffer-element-type.cpp`:
- Around line 2050-2062: getPointerTypeWithBufferLayout currently hardcodes
AccessQualifier::ReadWrite; change it to derive the qualifier from the buffer
type and pass that into builder.getPtrType. Inspect the IRType/IRInst opcode of
bufferType (e.g., kIROp_HLSLStructuredBufferType -> AccessQualifier::ReadOnly,
kIROp_HLSLRWStructuredBufferType -> AccessQualifier::ReadWrite) and use that
mapping (falling back to ReadWrite if unknown) instead of the hardcoded value;
then call builder.getPtrType(elementType, derivedQualifier,
loweringConfig.addressSpace, layoutType). Ensure the change is applied inside
getPointerTypeWithBufferLayout so all call sites get the correct qualifier.
In `@source/slang/slang-ir-spirv-legalize.cpp`:
- Around line 1147-1177: Both processStructuredBufferLoad and
processRWStructuredBufferStore create a pointer type for
kIROp_RWStructuredBufferGetElementPtr without propagating the source
StructuredBuffer's dataLayout, which can drop explicit Std140/Std430 layout
info; fix by extracting the dataLayout from the structured buffer type (e.g.,
via the StructuredBuffer IR type or
sb->getFullType()->getDataLayout()/getDataLayout()) and pass that layout into
builder.getPtrType when synthesizing the pointer type for the element, so the
new pointer created for the intrinsic preserves the original buffer's
dataLayout.
---
Nitpick comments:
In `@source/slang/slang-ir-insts.h`:
- Around line 3353-3363: The comment above getPtrType(IRType*, IRPtrTypeBase*)
incorrectly omits that data layout is also copied from oldPtrType; update the
comment to list that the op-type, access-qualifier, address-space, and
dataLayout are copied (and clarify the note about not reusing the same `inst`
for access-qualifier and address-space) so it accurately reflects the parameters
passed to the overloaded getPtrType call using oldPtrType->getOp(),
getAccessQualifier(), getAddressSpace(), and getDataLayout().
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
2 participants
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.
This is the implementation of the pointer data layout proposal shader-slang/spec#39. The patch adds a new generic parameter to
Ptr<T>that indicates the data layout used for storage ofTwhen accessed through that pointer. The default behavior (scalar layout) is retained unless a layout is explicitly specified. This parameter behaves similarly to the data layout parameter on buffer types (StructuredBuffer,ConstantBuffer).On the IR level,
IRPtrTypealso gains a new operand that indicates the data layout type. In addition to explicitly defined layouts, this operand is also used when lowering buffer accesses into pointers viaRWStructuredBufferGetElementPtr. This way, we don't lose the layout information when representing buffer data as a pointer in IR.Not sure if this is a breaking change, but just to be cautious, I marked it as such. I think breakage in user code is very unlikely, and no existing tests were broken.
The addition of layout info to pointers also unblocks creating a lowering pass that turns "opaque" resource types into concrete struct types for LLVM on CPU (#9478), but I'll leave that to a separate PR that I'll work on some time soon. (that's the thing I'm actually excited to work on, I kinda dragged my feet on this PR because this is less fun plumbing TBH)
Meta
I briefly attempted implementing the other approach suggested by @csyonghe. That got quite ugly inside the compiler, and I repeatedly ran into very sticky situations with it, so instead of spending more time on that, I went with the original proposal.
I'm not opposed to that approach, I quite like it in fact. But it's a lot of effort. I have a feeling that the data layout infrastructure really is in need of a larger overhaul anyway, it feels pretty messy right now. Such an overhaul would be the perfect opportunity to implement that approach.
I'm open to also taking a second look at implementing that approach if there's a clear route forward with it, it's just that I don't really want to spend many weeks of spare time implementing it. I'd rather want to spend my time improving the LLVM emitter.
So if we decide that the other approach is preferred, I'd suggest that the proposal in its current form is closed. Instead, the type layout parameter can be introduced on the IR side only, with users having no way to specify an explicit layout for a pointer. That way, we wouldn't build up even more legacy syntax, but I'd be able proceed with #9478.