Open
Conversation
📝 WalkthroughWalkthroughRefactors ray-tracing shader table handling to store per-pipeline PipelineData (per-entry raygen info and sizes), updates D3D12/Vulkan command paths to use that data (with bounds checks and early returns), adds vkCmdUpdateBuffer to device procs, and enables Vulkan ray-tracing tests by removing Vulkan-only skips. Changes
Sequence DiagramsequenceDiagram
actor App as Application
participant CR as CommandRecorder
participant ST as ShaderTable
participant CB as CommandBuffer
participant GPU as GPU_Driver
App->>CR: Prepare DispatchRays (rayGen index + params)
CR->>ST: getPipelineData(rayTracingPipeline)
ST-->>CR: PipelineData (buffer, raygenInfos, offsets/strides)
CR->>CR: Lookup raygenInfo by index
alt params present
CR->>CB: vkCmdUpdateBuffer(copy entryPointData -> SBT)
CB->>GPU: barrier / buffer visibility
end
CR->>CB: Configure SBT addresses & strides from PipelineData
App->>CB: vkCmdTraceRaysKHR (dispatch)
CB->>GPU: Execute ray tracing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
697-722:⚠️ Potential issue | 🔴 CriticalAdd
src/debug-layer/debug-pipeline.cppto CMakeLists.txt.The file
src/debug-layer/debug-pipeline.cppexists in the repository but is missing from the CMakeLists.txttarget_sourcesblock for the debug-layer component. This will prevent it from being compiled and linked into the library. Add it to the appropriate location in the debug-layer section of CMakeLists.txt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 697 - 722, The file src/debug-layer/debug-pipeline.cpp is present in the repo but not listed in the debug-layer target_sources block, so it never gets compiled; locate the debug-layer target_sources section in CMakeLists.txt (the block that adds sources for the debug-layer component) and add src/debug-layer/debug-pipeline.cpp to that list so the debug-layer object is built and linked into the library.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/d3d12/d3d12-command.cpp`:
- Around line 1107-1117: The code assumes
ShaderTableImpl::getPipelineData(m_rayTracingPipeline) always returns a valid
ShaderTableImpl::PipelineData and dereferences shaderTablePipelineData->buffer;
add a null check after calling
m_shaderTable->getPipelineData(m_rayTracingPipeline) and if it returns nullptr,
early-return (or set an error/abort path) to avoid dereferencing a null pointer.
Specifically, locate the block that assigns shaderTablePipelineData and
shaderTableAddr and guard usage of shaderTablePipelineData (and its buffer)
before computing m_rayGenTableAddr, m_rayGenRecordStride, and populating
m_dispatchRaysDesc so the function safely exits when PipelineData is missing.
In `@src/vulkan/vk-command.cpp`:
- Around line 1098-1115: The code assumes
m_shaderTable->getPipelineData(m_rayTracingPipeline) always returns non-null and
immediately dereferences m_shaderTablePipelineData->buffer; guard against
nullptr by checking the result of getPipelineData() (e.g.,
m_shaderTablePipelineData) and early-return or handle the error path if it's
null before accessing members like buffer, raygenTableSize, missRecordStride,
hitGroupRecordStride, and callableRecordStride; ensure any cleanup/logging is
performed and that m_shaderTablePipelineData is assigned only after the
null-check.
- Around line 1130-1169: The call to m_api.vkCmdUpdateBuffer must be guarded for
Vulkan constraints: validate that dstOffset (raygenInfo.sbtOffset) and copySize
(min(entryPointData.size, raygenInfo.paramsSize)) are non-zero, multiples of 4,
and that copySize <= 65536; if any check fails, do not call
m_api.vkCmdUpdateBuffer directly—either adjust/round the copy or use a fallback
(e.g., a staging buffer + vkCmdCopyBuffer or skip the update) so you avoid
violating vkCmdUpdateBuffer requirements; add these checks just before the
vkCmdUpdateBuffer call and handle the invalid-case path accordingly.
In `@src/vulkan/vk-shader-object-layout.cpp`:
- Around line 929-970: info.paramsSize is taken from
entryPointLayout->getTotalOrdinaryDataSize() before unwrapping
ConstantBuffer/PushConstant for raygen, which yields 0; for raygen compute
paramsSize by summing the inner element type ordinary data sizes from the
unwrapped ranges: iterate entryPointTypeLayout->getSubObjectRangeCount(), for
each range whose getBindingRangeType(...) is slang::BindingType::ConstantBuffer
or PushConstant get the wrapperLayout via getBindingRangeLeafTypeLayout(...),
get elementVarLayout = wrapperLayout->getElementVarLayout(), then add
elementVarLayout->getTypeLayout()->getTotalOrdinaryDataSize() into a paramsSize
accumulator; set info.paramsSize = that accumulated value for the raygen branch
(or compute and assign before storing info) so subsequent SBT parameter handling
uses the unwrapped size; keep non-raygen behavior using
entryPointLayout->getTotalOrdinaryDataSize().
In `@src/vulkan/vk-shader-object-layout.h`:
- Around line 405-408: Rename the member variable paramsSize to follow the m_
prefix convention (m_paramsSize) and update all references to it throughout the
codebase; change the declaration "size_t paramsSize = 0;" to "size_t
m_paramsSize = 0;" and modify any constructors, initializers, member accesses,
assignments, and usages (e.g., in methods of the same class/struct, functions
referencing this field, tests, and serialization/deserialization code) to use
m_paramsSize instead of paramsSize to keep naming consistent with the project's
member-variable convention.
In `@src/vulkan/vk-shader-object.h`:
- Around line 96-103: Rename the struct members to follow the m_ prefix and
camelCase and fix the typo: in struct EntryPointData change data -> m_data and
size -> m_size (also update the comment to "Size of the data in bytes"); update
all call sites that reference EntryPointData::data and ::size to use m_data and
m_size; likewise rename any class/struct members entryPointData and
entryPointCount to m_entryPointData and m_entryPointCount (references around the
EntryPointData usage at the later block that currently uses
entryPointData/entryPointCount must be updated to the new names); ensure
compilation by updating any variable initializers, constructors, or accessors
that touch these symbols.
In `@src/vulkan/vk-shader-table.h`:
- Line 48: Change the non-failing signature of getPipelineData to return a
Result and take an out-parameter for the PipelineData pointer so failures can be
propagated; i.e. replace PipelineData* getPipelineData(RayTracingPipelineImpl*
pipeline) with a Result getPipelineData(RayTracingPipelineImpl* pipeline,
PipelineData** outPipelineData), update the implementation to write the created
PipelineData to *outPipelineData and propagate any errors using
SLANG_RETURN_ON_FAIL where buffer or resource creation can fail, and update all
call sites to handle the Result and use the out-parameter accordingly.
- Around line 12-41: Rename the struct member fields to use the m_ prefix and
camelCase and update all call sites: in RaygenInfo change entryPointIndex,
sbtOffset, paramsSize, recordOffset, recordSize to m_entryPointIndex,
m_sbtOffset, m_paramsSize, m_recordOffset, m_recordSize; in PipelineData change
buffer, raygenInfos, raygenTableSize, missTableSize, hitTableSize,
callableTableSize, missRecordStride, hitGroupRecordStride, callableRecordStride
to m_buffer, m_raygenInfos, m_raygenTableSize, m_missTableSize, m_hitTableSize,
m_callableTableSize, m_missRecordStride, m_hitGroupRecordStride,
m_callableRecordStride; update all uses and initializers accordingly (search for
RaygenInfo:: and PipelineData:: usages and adjust in vk-shader-table.cpp,
vk-command.cpp and any other callers).
---
Outside diff comments:
In `@CMakeLists.txt`:
- Around line 697-722: The file src/debug-layer/debug-pipeline.cpp is present in
the repo but not listed in the debug-layer target_sources block, so it never
gets compiled; locate the debug-layer target_sources section in CMakeLists.txt
(the block that adds sources for the debug-layer component) and add
src/debug-layer/debug-pipeline.cpp to that list so the debug-layer object is
built and linked into the library.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CMakeLists.txtsrc/d3d12/d3d12-command.cppsrc/d3d12/d3d12-shader-table.cppsrc/vulkan/vk-api.hsrc/vulkan/vk-command.cppsrc/vulkan/vk-shader-object-layout.cppsrc/vulkan/vk-shader-object-layout.hsrc/vulkan/vk-shader-object.cppsrc/vulkan/vk-shader-object.hsrc/vulkan/vk-shader-table.cppsrc/vulkan/vk-shader-table.htests/test-ray-tracing-raygen-entrypoint.cpptests/test-ray-tracing-shader-table-multi-pipeline.cpp
💤 Files with no reviewable changes (2)
- tests/test-ray-tracing-raygen-entrypoint.cpp
- tests/test-ray-tracing-shader-table-multi-pipeline.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/vulkan/vk-command.cpp (1)
1135-1174:⚠️ Potential issue | 🟠 MajorGuard
vkCmdUpdateBufferagainst Vulkan size/offset constraints.
vkCmdUpdateBufferrequiresdstOffsetanddataSizeto be multiples of 4 anddataSize≤ 65536. Add validation (or fallback to a staging copy) to avoid validation errors or undefined behavior.🛠️ Suggested guard
VkDeviceSize dstOffset = raygenInfo.sbtOffset; VkDeviceSize copySize = std::min(entryPointData.size, raygenInfo.paramsSize); +if ((dstOffset % 4) != 0 || (copySize % 4) != 0 || copySize > 65536) +{ + // TODO: Use a staging buffer + vkCmdCopyBuffer for large/unaligned payloads. + SLANG_RHI_ASSERT_FAILURE("vkCmdUpdateBuffer size/offset constraints violated"); + return; +} m_api.vkCmdUpdateBuffer(Vulkan vkCmdUpdateBuffer valid usage requirements dstOffset dataSize multiple of 4 65536 limit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vulkan/vk-command.cpp` around lines 1135 - 1174, The vkCmdUpdateBuffer call must respect Vulkan constraints (dstOffset and dataSize multiples of 4 and dataSize ≤ 65536), so add a guard around m_api.vkCmdUpdateBuffer: compute dstOffset and copySize (from raygenInfo.sbtOffset and std::min(entryPointData.size, raygenInfo.paramsSize)) and if dstOffset % 4 == 0 && copySize % 4 == 0 && copySize <= 65536 call m_api.vkCmdUpdateBuffer as before; otherwise create a host-visible staging buffer, map and memcpy entryPointData.data into it, issue vkCmdCopyBuffer from the staging buffer to m_shaderTablePipelineData->buffer->m_buffer.m_buffer at dstOffset, and use the same pipeline barrier; update/clean up staging resources accordingly and reference symbols: vkCmdUpdateBuffer, m_api.vkCmdUpdateBuffer, dstOffset, copySize, entryPointData, raygenInfo, and m_shaderTablePipelineData->buffer to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/vulkan/vk-command.cpp`:
- Around line 1135-1174: The vkCmdUpdateBuffer call must respect Vulkan
constraints (dstOffset and dataSize multiples of 4 and dataSize ≤ 65536), so add
a guard around m_api.vkCmdUpdateBuffer: compute dstOffset and copySize (from
raygenInfo.sbtOffset and std::min(entryPointData.size, raygenInfo.paramsSize))
and if dstOffset % 4 == 0 && copySize % 4 == 0 && copySize <= 65536 call
m_api.vkCmdUpdateBuffer as before; otherwise create a host-visible staging
buffer, map and memcpy entryPointData.data into it, issue vkCmdCopyBuffer from
the staging buffer to m_shaderTablePipelineData->buffer->m_buffer.m_buffer at
dstOffset, and use the same pipeline barrier; update/clean up staging resources
accordingly and reference symbols: vkCmdUpdateBuffer, m_api.vkCmdUpdateBuffer,
dstOffset, copySize, entryPointData, raygenInfo, and
m_shaderTablePipelineData->buffer to locate the changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/d3d12/d3d12-command.cppsrc/vulkan/vk-command.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/d3d12/d3d12-command.cpp
skallweitNV
force-pushed
the
dev/skallweit/vulkan-raygen
branch
from
1a59d98 to
91e516b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/vulkan/vk-shader-object.h (1)
96-128:⚠️ Potential issue | 🟡 MinorNaming convention violations and typo in
EntryPointData(unchanged from previous review).
EntryPointDatamembersdataandsize, and theBindingDataImplmembersentryPointDataandentryPointCount, still lack the requiredm_prefix. Line 101 also retains the "bytess" typo. As per coding guidelines, member variables should start withm_prefix and be in camelCase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vulkan/vk-shader-object.h` around lines 96 - 128, Rename struct and class members to follow the m_ prefix and camelCase convention: in EntryPointData rename data -> m_data and size -> m_size (also fix the comment typo "bytess" -> "bytes"), and in BindingDataImpl (or the owning class) rename entryPointData -> m_entryPointData and entryPointCount -> m_entryPointCount; update all references/usages (constructors, initializers, accessors) to use the new names to keep the API consistent.src/vulkan/vk-shader-table.h (2)
12-41:⚠️ Potential issue | 🟡 Minor
RaygenInfoandPipelineDatamember variables still lackm_prefix.All fields in both structs (
entryPointIndex,sbtOffset,paramsSize,recordOffset,recordSize,buffer,raygenInfos,raygenTableSize, etc.) should use them_prefix per the project naming convention. As per coding guidelines, member variables should start withm_prefix and be in camelCase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vulkan/vk-shader-table.h` around lines 12 - 41, Update the member field names in struct RaygenInfo and struct PipelineData to follow the project naming convention by adding the m_ prefix and using camelCase: rename RaygenInfo::entryPointIndex, sbtOffset, paramsSize, recordOffset, recordSize to m_entryPointIndex, m_sbtOffset, m_paramsSize, m_recordOffset, m_recordSize; and rename PipelineData::buffer, raygenInfos, raygenTableSize, missTableSize, hitTableSize, callableTableSize, missRecordStride, hitGroupRecordStride, callableRecordStride to m_buffer, m_raygenInfos, m_raygenTableSize, m_missTableSize, m_hitTableSize, m_callableTableSize, m_missRecordStride, m_hitGroupRecordStride, m_callableRecordStride; then update all uses/references of these symbols throughout the codebase (constructors, accessors, member initializers, and any external files) to the new names.
48-48: 🛠️ Refactor suggestion | 🟠 Major
getPipelineDatashould returnResultwith an out-parameter.The implementation can fail (buffer creation failure), but the current signature returns a raw nullable pointer. Per coding guidelines, functions that can fail should return a
Resulttype and output values via pointer parameters.🔧 Suggested signature change
- PipelineData* getPipelineData(RayTracingPipelineImpl* pipeline); + Result getPipelineData(RayTracingPipelineImpl* pipeline, PipelineData** outPipelineData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vulkan/vk-shader-table.h` at line 48, Change the getPipelineData(RayTracingPipelineImpl* pipeline) API to return a Result and provide the PipelineData via an out-parameter (e.g., Result getPipelineData(RayTracingPipelineImpl* pipeline, PipelineData** outData) or Result getPipelineData(RayTracingPipelineImpl* pipeline, PipelineData*& outData)); update the declaration and the implementation in vk-shader-table to allocate/populate the PipelineData into the out parameter and return an error Result when buffer creation or any other failure occurs instead of returning nullptr; ensure all callers are updated to check the returned Result and use the out parameter on success.src/vulkan/vk-shader-object-layout.cpp (1)
935-963:⚠️ Potential issue | 🟠 Major
paramsSizewill be 0 for raygen entry points with uniform parameters.
entryPointLayout->getTotalOrdinaryDataSize()returnsm_totalOrdinaryDataSize, which was set insetElementTypeLayoutvia_unwrapParameterGroups(entryPointLayout->getTypeLayout(), ...). For a Vulkan raygen entry point, Slang'sgetTypeLayout()returns a struct whose uniform params are wrapped insideConstantBuffer<T>(orPushConstant) sub-objects._unwrapParameterGroupsonly unwraps when the top-level type is a parameter group — it does not dive into sub-object ranges. Sostruct->getSize() == 0, meaningm_totalOrdinaryDataSize == 0andparamsSize == 0. The raygen-specific loop (lines 950–963) correctly unwraps for descriptor registration but never updatesparamsSize, so the SBT record size stayshandleSizeand the parameter copy path incmdDispatchRaysis never triggered.To fix, compute
paramsSizefrom the element type inside the unwrapped wrapper, as suggested previously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vulkan/vk-shader-object-layout.cpp` around lines 935 - 963, The paramsSize remains zero for raygen entry points because entryPointLayout->getTotalOrdinaryDataSize() is computed from the top-level type (a struct) while the actual uniform params live inside ConstantBuffer/PushConstant sub-objects; update the ray-generation branch (where slangEntryPointLayout->getStage() == SLANG_STAGE_RAY_GENERATION) to compute and assign info.paramsSize from the unwrapped element type layout instead of relying on entryPointLayout->getTotalOrdinaryDataSize(): inside the loop that inspects sub-object ranges (using entryPointTypeLayout, bindingRangeIndex, wrapperLayout, elementVarLayout) obtain the inner element type layout (elementVarLayout->getTypeLayout()), compute its ordinary data size (equivalent to what getTotalOrdinaryDataSize would return for that element type) and add/assign that to info.paramsSize so the SBT record size reflects the wrapped uniform parameter size and the cmdDispatchRays copy path will be taken.src/vulkan/vk-command.cpp (1)
1149-1157:⚠️ Potential issue | 🟠 Major
vkCmdUpdateBufferinvoked without validating alignment and size constraints.The Vulkan spec requires:
dstOffsetmust be a multiple of 4dataSizemust be a multiple of 4dataSize≤ 65536 bytes
copySize = std::min(entryPointData.size, raygenInfo.paramsSize)is used directly with no alignment padding or upper-bound guard. If a parameter struct is not naturally 4-byte-aligned in size (e.g.,paramsSize = 5), or if it exceeds 65536 bytes, this call will trigger Vulkan validation errors or undefined behaviour.For the size-limit case, a fallback to a staging buffer +
vkCmdCopyBufferis needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vulkan/vk-command.cpp` around lines 1149 - 1157, vkCmdUpdateBuffer is being called with dstOffset and copySize without enforcing Vulkan's alignment and size constraints; ensure dstOffset (raygenInfo.sbtOffset) and dataSize (copySize computed from entryPointData.size and raygenInfo.paramsSize) are rounded up to a 4-byte multiple and that dataSize <= 65536. Modify the code around copySize and the vkCmdUpdateBuffer invocation to: compute an alignedSize = align_up(std::min(entryPointData.size, raygenInfo.paramsSize), 4), clamp to 65536, and if the needed size exceeds 65536 or the source buffer cannot provide aligned data, copy the full parameter payload into a temporary 4-byte-aligned staging buffer and perform a vkCmdCopyBuffer to m_shaderTablePipelineData->buffer->m_buffer.m_buffer via m_cmdBuffer instead of calling m_api.vkCmdUpdateBuffer; otherwise call m_api.vkCmdUpdateBuffer with dstOffset aligned and the alignedSize.
🧹 Nitpick comments (2)
src/d3d12/d3d12-command.cpp (1)
1120-1120: Dead write:RayGenerationShaderRecord.StartAddressis unconditionally overridden before use.Line 1120 sets
StartAddress = shaderTableAddr(the raw buffer base), butcmdDispatchRaysoverwrites it with the correct per-entry address before callingDispatchRays. The assignment here is never observed by the GPU.♻️ Proposed cleanup
- m_dispatchRaysDesc.RayGenerationShaderRecord.StartAddress = shaderTableAddr; m_dispatchRaysDesc.RayGenerationShaderRecord.SizeInBytes = shaderTablePipelineData->rayGenRecordStride;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/d3d12/d3d12-command.cpp` at line 1120, Dead write: remove the unconditional assignment to m_dispatchRaysDesc.RayGenerationShaderRecord.StartAddress = shaderTableAddr since cmdDispatchRays overwrites the start address per-entry before calling DispatchRays; locate the assignment by the symbol m_dispatchRaysDesc and the field RayGenerationShaderRecord.StartAddress and either delete that line or guard it so it is only set when a true base address is required (ensure cmdDispatchRays still computes and writes the correct per-entry address before DispatchRays).src/vulkan/vk-shader-table.cpp (1)
17-32: Remove unusedlayoutparameter fromfindEntryPointIndexByName.The
RootShaderObjectLayoutImpl* layoutparameter is never referenced in the function body — onlyprogramLayoutandnameare used. The parameter can be dropped, and call sites updated accordingly.♻️ Proposed simplification
-static uint32_t findEntryPointIndexByName( - RootShaderObjectLayoutImpl* layout, - slang::ProgramLayout* programLayout, - const std::string& name -) +static uint32_t findEntryPointIndexByName( + slang::ProgramLayout* programLayout, + const std::string& name +)Update the call site:
-uint32_t entryPointIndex = findEntryPointIndexByName(rootLayout, programLayout, entryPointName); +uint32_t entryPointIndex = findEntryPointIndexByName(programLayout, entryPointName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vulkan/vk-shader-table.cpp` around lines 17 - 32, The function findEntryPointIndexByName currently declares an unused parameter RootShaderObjectLayoutImpl* layout; remove that parameter from the function signature and definition so it becomes findEntryPointIndexByName(slang::ProgramLayout* programLayout, const std::string& name), then update every call site to pass only programLayout and name (drop the layout argument). Also update any forward declarations, prototypes, or header entries for findEntryPointIndexByName to match the new signature and rebuild to catch remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/vulkan/vk-command.cpp`:
- Around line 1149-1157: vkCmdUpdateBuffer is being called with dstOffset and
copySize without enforcing Vulkan's alignment and size constraints; ensure
dstOffset (raygenInfo.sbtOffset) and dataSize (copySize computed from
entryPointData.size and raygenInfo.paramsSize) are rounded up to a 4-byte
multiple and that dataSize <= 65536. Modify the code around copySize and the
vkCmdUpdateBuffer invocation to: compute an alignedSize =
align_up(std::min(entryPointData.size, raygenInfo.paramsSize), 4), clamp to
65536, and if the needed size exceeds 65536 or the source buffer cannot provide
aligned data, copy the full parameter payload into a temporary 4-byte-aligned
staging buffer and perform a vkCmdCopyBuffer to
m_shaderTablePipelineData->buffer->m_buffer.m_buffer via m_cmdBuffer instead of
calling m_api.vkCmdUpdateBuffer; otherwise call m_api.vkCmdUpdateBuffer with
dstOffset aligned and the alignedSize.
In `@src/vulkan/vk-shader-object-layout.cpp`:
- Around line 935-963: The paramsSize remains zero for raygen entry points
because entryPointLayout->getTotalOrdinaryDataSize() is computed from the
top-level type (a struct) while the actual uniform params live inside
ConstantBuffer/PushConstant sub-objects; update the ray-generation branch (where
slangEntryPointLayout->getStage() == SLANG_STAGE_RAY_GENERATION) to compute and
assign info.paramsSize from the unwrapped element type layout instead of relying
on entryPointLayout->getTotalOrdinaryDataSize(): inside the loop that inspects
sub-object ranges (using entryPointTypeLayout, bindingRangeIndex, wrapperLayout,
elementVarLayout) obtain the inner element type layout
(elementVarLayout->getTypeLayout()), compute its ordinary data size (equivalent
to what getTotalOrdinaryDataSize would return for that element type) and
add/assign that to info.paramsSize so the SBT record size reflects the wrapped
uniform parameter size and the cmdDispatchRays copy path will be taken.
In `@src/vulkan/vk-shader-object.h`:
- Around line 96-128: Rename struct and class members to follow the m_ prefix
and camelCase convention: in EntryPointData rename data -> m_data and size ->
m_size (also fix the comment typo "bytess" -> "bytes"), and in BindingDataImpl
(or the owning class) rename entryPointData -> m_entryPointData and
entryPointCount -> m_entryPointCount; update all references/usages
(constructors, initializers, accessors) to use the new names to keep the API
consistent.
In `@src/vulkan/vk-shader-table.h`:
- Around line 12-41: Update the member field names in struct RaygenInfo and
struct PipelineData to follow the project naming convention by adding the m_
prefix and using camelCase: rename RaygenInfo::entryPointIndex, sbtOffset,
paramsSize, recordOffset, recordSize to m_entryPointIndex, m_sbtOffset,
m_paramsSize, m_recordOffset, m_recordSize; and rename PipelineData::buffer,
raygenInfos, raygenTableSize, missTableSize, hitTableSize, callableTableSize,
missRecordStride, hitGroupRecordStride, callableRecordStride to m_buffer,
m_raygenInfos, m_raygenTableSize, m_missTableSize, m_hitTableSize,
m_callableTableSize, m_missRecordStride, m_hitGroupRecordStride,
m_callableRecordStride; then update all uses/references of these symbols
throughout the codebase (constructors, accessors, member initializers, and any
external files) to the new names.
- Line 48: Change the getPipelineData(RayTracingPipelineImpl* pipeline) API to
return a Result and provide the PipelineData via an out-parameter (e.g., Result
getPipelineData(RayTracingPipelineImpl* pipeline, PipelineData** outData) or
Result getPipelineData(RayTracingPipelineImpl* pipeline, PipelineData*&
outData)); update the declaration and the implementation in vk-shader-table to
allocate/populate the PipelineData into the out parameter and return an error
Result when buffer creation or any other failure occurs instead of returning
nullptr; ensure all callers are updated to check the returned Result and use the
out parameter on success.
---
Nitpick comments:
In `@src/d3d12/d3d12-command.cpp`:
- Line 1120: Dead write: remove the unconditional assignment to
m_dispatchRaysDesc.RayGenerationShaderRecord.StartAddress = shaderTableAddr
since cmdDispatchRays overwrites the start address per-entry before calling
DispatchRays; locate the assignment by the symbol m_dispatchRaysDesc and the
field RayGenerationShaderRecord.StartAddress and either delete that line or
guard it so it is only set when a true base address is required (ensure
cmdDispatchRays still computes and writes the correct per-entry address before
DispatchRays).
In `@src/vulkan/vk-shader-table.cpp`:
- Around line 17-32: The function findEntryPointIndexByName currently declares
an unused parameter RootShaderObjectLayoutImpl* layout; remove that parameter
from the function signature and definition so it becomes
findEntryPointIndexByName(slang::ProgramLayout* programLayout, const
std::string& name), then update every call site to pass only programLayout and
name (drop the layout argument). Also update any forward declarations,
prototypes, or header entries for findEntryPointIndexByName to match the new
signature and rebuild to catch remaining references.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CMakeLists.txtsrc/d3d12/d3d12-command.cppsrc/d3d12/d3d12-shader-table.cppsrc/vulkan/vk-api.hsrc/vulkan/vk-command.cppsrc/vulkan/vk-shader-object-layout.cppsrc/vulkan/vk-shader-object-layout.hsrc/vulkan/vk-shader-object.cppsrc/vulkan/vk-shader-object.hsrc/vulkan/vk-shader-table.cppsrc/vulkan/vk-shader-table.htests/test-ray-tracing-raygen-entrypoint.cpptests/test-ray-tracing-shader-table-multi-pipeline.cpp
💤 Files with no reviewable changes (2)
- tests/test-ray-tracing-raygen-entrypoint.cpp
- tests/test-ray-tracing-shader-table-multi-pipeline.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/d3d12/d3d12-shader-table.cpp
- CMakeLists.txt
- src/vulkan/vk-shader-object.cpp
- src/vulkan/vk-api.h
- src/vulkan/vk-shader-object-layout.h
Collaborator
Author
|
@csyonghe can you take a look at the changes in the vk shader object and layout changes? |
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
1 participant
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.
Summary by CodeRabbit