Update from task a6ea5542-24e3-4e5b-ad34-e79b70444f85#30
Open
tronpis wants to merge 2 commits into
Open
Conversation
- Extended RenderGraphBuilder::addDependency and addResourceDependency with optional VkDependencyFlags parameter - Modified RenderGraph::compile to default PassLevelDependency::dependencyFlags to 0 instead of VK_DEPENDENCY_BY_REGION_BIT - Updated RenderGraph::recordBarriers to emit separate vkCmdPipelineBarrier calls when memory and image barriers have different dependency flags - Added dependencyFlags field to PassDependency, PendingResourceBarrier and PassLevelDependency structs - Fixed barrier emission logic to prevent BY_REGION flag from incorrectly applying to all barrier types in same call This change provides explicit control over dependency flags while maintaining safe defaults and ensuring correct synchronization behavior for different resource types.
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoFix VK_DEPENDENCY_BY_REGION_BIT overuse in RenderGraph barriers
WalkthroughsDescription• Fixed VK_DEPENDENCY_BY_REGION_BIT overuse by adding explicit VkDependencyFlags parameter to dependency methods • Modified barrier emission logic to use separate vkCmdPipelineBarrier calls when memory and image barriers have different dependency flags • Changed PassLevelDependency default dependencyFlags from VK_DEPENDENCY_BY_REGION_BIT to 0 for safer defaults • Added dependencyFlags field to PassDependency, PendingResourceBarrier, and PassLevelDependency structs Diagramflowchart LR
A["addDependency<br/>addResourceDependency"] -->|"add dependencyFlags<br/>parameter"| B["PassDependency struct"]
B -->|"propagate flags"| C["PendingResourceBarrier<br/>PassLevelDependency"]
C -->|"separate barriers<br/>if flags differ"| D["vkCmdPipelineBarrier calls"]
E["Default: 0<br/>not BY_REGION_BIT"] -->|"safer defaults"| D
File Changes1. FarmEngine/src/rendering/graph/RenderGraph.h
|
Code Review by Qodo
1. Per-dependency flags lost
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR correctly addresses Vulkan barrier semantics by removing the hardcoded VK_DEPENDENCY_BY_REGION_BIT and allowing proper per-dependency flag control. The implementation properly separates memory and image barrier dependency flags to prevent unintended flag propagation.
Critical Issue Found:
- Line 469: Logic error in ternary operator could select wrong dependency flags when both barrier types exist
Changes Validated:
- API signature updates for
addDependencyandaddResourceDependencywith optionaldependencyFlagsparameter - Proper propagation of dependency flags through compilation pipeline
- Separate tracking of memory vs image barrier flags to prevent cross-contamination
Please address the critical finding before merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+382
to
386
| // Combine dependency flags from all pass-level dependencies (for memory barriers only) | ||
| VkDependencyFlags combinedMemoryDependencyFlags = 0; | ||
|
|
||
| // Process pass-level dependencies (emit as VkMemoryBarrier) | ||
| std::vector<VkMemoryBarrier> memoryBarriers; |
There was a problem hiding this comment.
1. Per-dependency flags lost 🐞 Bug ≡ Correctness
RenderGraph::recordBarriers bitwise-ORs dependencyFlags across all memory barriers and across all image barriers, then applies the combined value to a single vkCmdPipelineBarrier call. This makes it impossible to use different VkDependencyFlags for different dependencies of the same barrier type because a flag set by one dependency is applied to all barriers in that batch.
Agent Prompt
### Issue description
`RenderGraph::recordBarriers()` currently merges `VkDependencyFlags` via bitwise-OR across all memory barriers and across all image barriers, then uses that single value for a batched `vkCmdPipelineBarrier()` call. Since `vkCmdPipelineBarrier()` takes one `dependencyFlags` value per call, this causes any flag requested by one dependency to be applied to all barriers in that batch, defeating the intent of the new per-dependency `dependencyFlags` API.
### Issue Context
The builder now accepts `dependencyFlags` per dependency, so the runtime barrier emission should respect heterogeneous flags.
### Fix Focus Areas
- FarmEngine/src/rendering/graph/RenderGraph.cpp[377-482]
### Suggested approach
1. Group barriers by `VkDependencyFlags` value (at least within each barrier type):
- Build `std::unordered_map<VkDependencyFlags, Group>` where `Group` contains:
- `std::vector<VkMemoryBarrier> memoryBarriers`
- `std::vector<VkImageMemoryBarrier> imageBarriers`
- `VkPipelineStageFlags srcStages`, `dstStages`
2. For each pass-level dependency, append to `groups[dep.dependencyFlags].memoryBarriers` and OR its stage masks into that group's stage masks.
3. For each pending resource barrier, append to `groups[pending.dependencyFlags].imageBarriers` and OR its stage masks into that group's stage masks.
4. Iterate groups and emit **one `vkCmdPipelineBarrier()` per flags value**, using that group’s stage masks (with the existing ALL_COMMANDS fallback per group).
This preserves per-dependency flag semantics while still batching where possible.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the project's .gitignore and enhances the Vulkan RenderGraph to support explicit VkDependencyFlags. The RenderGraph now correctly handles cases where memory and image barriers have different dependency flags by splitting them into separate vkCmdPipelineBarrier calls. A syntax error was identified in the .gitignore file where markdown code block markers were incorrectly added to the top of the file.
| @@ -1,33 +1,30 @@ | |||
| ``` | |||
| # Compiled and binary files | |||
| ```cpp | |||
Contributor
There was a problem hiding this comment.
The .gitignore file contains markdown code block syntax (```cpp). This is invalid for a .gitignore file and should be removed. Git will interpret this line as a literal pattern to match.
# Compiled and build artifacts
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 PR was created by qwen-chat coder for task a6ea5542-24e3-4e5b-ad34-e79b70444f85.