-
Notifications
You must be signed in to change notification settings - Fork 61
Add an option to the replayer to fail pipeline creation if caching didn't prevent compilation #277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Passing this option will add `VK_PIPELINE_CREATE_FAIL_ON_PIPELINE_COMPILE_REQUIRED_BIT_EXT` flag while creating graphics, compute and raytracing pipelines.
@Themaister can this be merged? |
I didn't see the PR before now. I'll look at it. |
nullptr, work_item.output.pipeline) == VK_SUCCESS) | ||
VkResult result = vkCreateGraphicsPipelines(device->get_device(), cache, 1, work_item.create_info.graphics_create_info, | ||
nullptr, work_item.output.pipeline); | ||
if (result == VK_SUCCESS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If COMPILE_REQUIRED returns false, you'll need to recompile it without the flag. Other PSOs may depend on this PSO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there are considerations to be made for GPL pipelines. It's likely that fast-linked PSOs are never placed in caches, so it's possible anything related to GPL/pipeline libraries have to be counted separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the replay is purely intended as some kind of assertion that compilation is working, it's fine to add another option that forces missed PSOs to actually fail completely, but I don't think it should be the default.
@@ -3462,6 +3495,10 @@ struct ThreadedReplayer : StateCreatorInterface | |||
std::atomic<std::uint32_t> pipeline_cache_hits; | |||
std::atomic<std::uint32_t> pipeline_cache_misses; | |||
|
|||
std::atomic<std::uint32_t> graphics_pipeline_compile_required_count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose only the plain replayer (no --progress) is supported here? I guess that's fine for this PR.
@@ -4094,7 +4094,8 @@ bool FeatureFilter::Impl::graphics_pipeline_is_supported(const VkGraphicsPipelin | |||
VK_PIPELINE_CREATE_2_INDIRECT_BINDABLE_BIT_EXT | | |||
VK_PIPELINE_CREATE_2_DISALLOW_OPACITY_MICROMAP_BIT_ARM | | |||
VK_PIPELINE_CREATE_2_RAY_TRACING_ALLOW_SPHERES_AND_LINEAR_SWEPT_SPHERES_BIT_NV | | |||
VK_PIPELINE_CREATE_2_PER_LAYER_FRAGMENT_DENSITY_BIT_VALVE; | |||
VK_PIPELINE_CREATE_2_PER_LAYER_FRAGMENT_DENSITY_BIT_VALVE | | |||
VK_PIPELINE_CREATE_FAIL_ON_PIPELINE_COMPILE_REQUIRED_BIT_EXT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 2 flags here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some concerns about not actually compiling the PSOs, but seems fine otherwise.
I'll just make the changes myself I think since I took so long to respond. |
Adds a new command line option
--fail-on-pipeline-compile-required
tofossilize-replay.exe
.It adds flag
VK_PIPELINE_CREATE_FAIL_ON_PIPELINE_COMPILE_REQUIRED_BIT_EXT
to the create info struct of all pipeline creations, and keeps the track of pipelines that require compile.Closes #276.