-
Notifications
You must be signed in to change notification settings - Fork 419
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
gpu: Remove passthrough to run a Pass #9202
gpu: Remove passthrough to run a Pass #9202
Conversation
CI Vulkan-ValidationLayers build queued with queue ID 342384. |
7615a59
to
02fb1a0
Compare
CI Vulkan-ValidationLayers build queued with queue ID 342399. |
CI Vulkan-ValidationLayers build # 18609 running. |
CI Vulkan-ValidationLayers build # 18609 passed. |
02fb1a0
to
928e3e6
Compare
CI Vulkan-ValidationLayers build queued with queue ID 342649. |
CI Vulkan-ValidationLayers build # 18615 running. |
CI Vulkan-ValidationLayers build # 18615 passed. |
@@ -113,8 +103,7 @@ class Module { | |||
const uint32_t output_buffer_descriptor_set_; | |||
|
|||
const bool support_non_semantic_info_; |
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.
should support_non_semantic_info_
be removed too?
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.
this is from an extension, was debating to do it, but doubt other passes care about extension, but 100% will care about feature bits
layers/gpuav/spirv/pass.h
Outdated
// Require passes to print info as extremely helpful for debugging | ||
virtual void PrintDebugInfo() const = 0; | ||
// Wrapper that each pass can use to start | ||
bool Start(); |
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.
a Start
calls for a End
or a Finish
, so IMO inappropriate here.
If I follow things correctly, Run
is now only called within Start
.
So I would expect calls now looking like modified |= texel_buffer_pass.Start();
to stay similar to how they were, by going modified |= texel_buffer_pass.Run();
And here Run
would look like your Start
:
bool Pass::Run() {
// I too would like to have "Instrument" be called `Run`, but since we can't I guess we can only cope
// Also "Instrument" should probably be a `protected` method
const bool modified = Instrument();
if (module_.print_debug_info_) {
PrintDebugInfo();
}
return modified;
}
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.
the Run()
is what spirv-opt
uses to "run a pass"
honestly Run
and Instrument
are better... I just picked Start
because I needed a name
928e3e6
to
1a8bfda
Compare
CI Vulkan-ValidationLayers build queued with queue ID 344603. |
CI Vulkan-ValidationLayers build # 18645 running. |
CI Vulkan-ValidationLayers build # 18645 failed. |
CI Vulkan-ValidationLayers build queued with queue ID 344715. |
CI Vulkan-ValidationLayers build # 18649 running. |
CI Vulkan-ValidationLayers build # 18649 passed. |
We use to add this
type logic for every pass we added, removed it (and just passed in
enabled_features
) so it is more streamline to quickly/easily add a new SPIR-V pass