Vulkan bandwidth optimizations (configure renderpass load/store better) #15917
Conversation
| switch (action) { | ||
| case VKRRenderPassLoadAction::CLEAR: return VK_ATTACHMENT_LOAD_OP_CLEAR; | ||
| case VKRRenderPassLoadAction::KEEP: return VK_ATTACHMENT_LOAD_OP_LOAD; | ||
| default: return VK_ATTACHMENT_LOAD_OP_DONT_CARE; |
There was a problem hiding this comment.
nit: I feel like you've switched to doing this recently, and it's very anti-defensive-programming.
Let's say we have CLEAR, KEEP, DONT_CARE, but Vulkan 2.0 comes out and we add KEEP_OR_CLEAR_IF_CHEAPER (I know this is unlikely, but things happen.)
The intent of compiler warnings is that they'd now notify you, "oh no, this switch doesn't include a case for KEEP_OR_CLEAR_IF_CHEAPER, and the two parts of code that are in different places are misaligned." Including a default says you don't want this help from the compiler, which I already disagree with in usual cases.
Replacing one case with default just seems even worse. Now a grep or search for VKRRenderPassLoadAction::DONT_CARE won't even find it, plus confusion is likely if a new value is added. It may even become unclear, later, whether the default intended to cover both the values or not until someone checks dates. Like if (storeOp == STORE) { ... } else { ...} assumptions, the same.
Personally I think the better pattern is:
switch (foo) {
case A: return 1;
case B: return 2;
case C: return 3;
}
return 1;
Probably load/store actions will never change, I've just been seeing it more often so saying something.
-[Unknown]
There was a problem hiding this comment.
No I'm very aware of the benefits of fully specifying all the cases and then letting the compiler warn, it's just that in C++ it's too loose.
I've gotten used to the Rust compiler forcing me to do it and allowing me to safely not specifying a default, while back in C++ land where everything is looser and the compiler will warn if I don't have a default case, I've started resorting to this.
I agree that it's bad, and having a separate return after the switch is a pretty good solution that I didn't consider.
This makes a few straightforward optimizations to when we load/store from framebuffers (in depal, for example, can ignore depth/stencil entirely when drawing to temp FBO, and limit the drawn rectangle in the renderpass).
In reinterpret, by adding a check for full coverage in BlitUsingRaster we know that previous contents of the framebuffer will be irrelevant so can safely set the current render pass to loadop = DONT_CARE.
There's more to be done here later, mainly we should be able to create color-only renderpasses when possible, so we don't even have to consider the depth channel.