WIP: Transform feedback #1943
WIP: Transform feedback #1943
Conversation
As of macOS Big Sur and iOS/tvOS 14, the `discard_fragment()` function in MSL is defined to have demote semantics; that is, fragment shader output is discarded, but the fragment shader thread continues to run as a helper invocation. This is very useful for Direct3D emulation, since this is the semantic that HLSL `discard` has. Signed-off-by: Chip Davis <chip@holochip.com>
…pple Silicon. Use an explicit gradient to make it sample the correct level. Update SPIRV-Cross to pull in the change needed for this.
Work around problems with explicit LoD with arrayed depth images on Apple Silicon. Approved-by: Steven Winston
As of macOS Big Sur and iOS/tvOS 14, the `discard_fragment()` function in MSL is defined to have demote semantics; that is, fragment shader output is discarded, but the fragment shader thread continues to run as a helper invocation. This is very useful for Direct3D emulation, since this is the semantic that HLSL `discard` has. Signed-off-by: Chip Davis <chip@holochip.com>
Advertise the VK_EXT_shader_demote_to_helper_invocation extension.
# Conflicts: # Docs/MoltenVK_Runtime_UserGuide.md # MoltenVK/MoltenVK/API/vk_mvk_moltenvk.h # MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm # MoltenVK/MoltenVK/GPUObjects/MVKRenderPass.mm # MoltenVK/MoltenVK/Layers/MVKExtensions.def
|
You should probably mark this as a draft for now. |
# Conflicts: # Docs/MoltenVK_Runtime_UserGuide.md # MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm # MoltenVK/MoltenVK/Layers/MVKExtensions.def
# Conflicts: # MoltenVK/MoltenVK/API/vk_mvk_moltenvk.h # MoltenVK/MoltenVK/GPUObjects/MVKRenderPass.mm
…dating from upstream/main.
…r time. 2.) remove validation from the driver layer.
Leave some room for XFB buffers and the argument buffers by limiting maximum vertex input bindings to 16.
# Conflicts: # Docs/MoltenVK_Runtime_UserGuide.md # ExternalRevisions/SPIRV-Cross_repo_revision # MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm # MoltenVK/MoltenVK/Layers/MVKExtensions.def
reverting the revision change.
…o transform_feedback_public # Conflicts: # ExternalRevisions/SPIRV-Cross_repo_revision
We still need to emit the array of clip distances for the rasterizer; this is just for the fragment shader interface.
Fix clip/cull distance attribute for the fragment shader interface.
Make sure SPIRV-Cross knows about them.
|
It concerns me that this PR has been marked “ready”, as the technical issues raised previously do not seem to be addressed (or acknowledged). From a cursory review, the issues with this implementation approach are not gotchas, but fundamental holes large enough to drive a truck through. Are you sure this PR is ready? |
|
Hi, yes, this PR is correctly marked as "ready" for review. We eagerly await hearing any identification of anything not working in this PR during a review. That's exactly how the process is meant to play out. We aim to ensure transform feedback works with the full Vulkan CTS tests that cover transform feedback functionality specified in the Vulkan Spec. Things that don't work, can be worked on in the future if they exist beyond the scope of this PR effort. If you have any constructive ideas of anything not working with this approach, that would be helpful to hear. If you can identify better approaches; that would be great to hear, all of that is correctly a part of review for candidate submissions and we welcome the chance to hear about it. It's also especially great to hear where bugs in the code are from rigorous testing. |
|
The CMake changes will be split from this PR. It is already in a separate PR I need to update when I find the time. |
|
@gpx1000, instead of being being coy and evasive, you could just address the issues that Alyssa already raised. Why not indicate what is and what is not within the scope of your project? It would be much more useful for everyone else to estimate if this PR is actually addressing their needs, or just providing a partial solution. |
Tessellation plus transform feedback is definitely broken here, and cannot be fixed without a fundamental re-architecture of both this transform feedback implementation, and MoltenVK’s tessellation. This is not a “portability” issue, it is surely possible to implement these features correctly on top of metal, but that’s not what’s done here. I would be shocked if CTS does not test this, the GLES CTS certainly does. Geometry shaders plus transform feedback is going to be broken unless the transform feedback code gets rewritten when geometry shaders are introduced. It concerns me that that code is also seeking review right now, given its own set of brokenness, for example, around tessellation (again, geometry, shaders, must work with tessellation, and that cannot be made to work without re-writing MoltenVK’s tessellation implementation, which is unsuitable. Unfortunately, I don’t think you can even keep the existing Tess implementation as a fast path, because the APIs require invariance between similar tessellation invocations, which would effectively require the compute/mesh tessellator to match the Metal tessellator exactly. as the behaviour of medals tessellation is undocumented and presumably varies between hardware, that would be extremely difficult for a reverse, engineered, native driver to do and a non-starter for a layered implementation that also needs to be correct on eg Intel and AMD metal drivers.) As Hans Kristian pointed out in the associated SPIRV PR, the “shade from the XFB buffers” approach is broken, because the XFB buffers are allowed to overflow without affecting rasterization. Transform feedback in conjunction with primitive restart is tricky and I do not see any code in either PR that would handle this interaction correctly. This is especially relevant for moltenvk because Metal always uses primitive restart. These are just a few of the issues raised weeks ago. |
|
The feature set currently supported by the PR is meant to be within scope for this PR. It would make sense in the future to create a new PR to address tessellation shaders. It equally would make sense to start looking at how to get geometry shader support in transform feedback after geometry support is in MoltenVK. |
If I understand Alyssa's concern correctly, this code is not extensible and will have to be rewritten to accommodate further changes like geometry shaders, in which case, it doesn't make sense to merge a half-baked implementation now, to rework it all down the line. If this feature is added it should be added completely and correctly. |
MoltenVK advertises tessellation already, which means transform feedback must work with tessellation. This is core to transform feedback, and in a PR titled “Transform feedback”, it is absolutely in scope. Indeed, this interaction is the very reason that transform feedback exists in Vulkan, instead of requiring applications to simply use storage buffers. See https://www.gfxstrand.net/faith/blog/2018/10/transform-feedback-is-terrible-so-why/ There are two acceptable options if you want this code to be merged: disable tessellation in MoltenVK, or address the interaction. Correctness is not optional. Conformance is not optional. There is no portability case to be made here. Given this entire feature is emulated, there is no excuse not to emulate it properly. If you believe that it is impossible to emulate correctly on top of Metal, then MoltenVK has no business implementing it at all — after all, transform feedback is an optional legacy wart on Vulkan. As the linked article explains, new Vulkan code and even Vulkan ports of existing games should not use this functionality. So adding this support does not advance MoltenVK’s mission of aiding portability. Neither can bugs in this support get overlooked in the name of portability. Even if you don’t care about Vulkan, DX11 requires these features, and your users will run into trouble trying to use this code with DXVK running arbitrary games. Correctness matters. |
|
I'm all for adding new functionality within future PRs. However, the work here is reflective of the scoped out work decisions made during development. Nothing here prevents future work, nor does anything here do more than add functionality to moltenvk that it currently does not have. |
By whom were those decisions made? Did you check with the maintainers of MoltenVK to see if that would be accepted? Worth noting that the SPIRV-Cross side of this PR has had unattended feedback since September. If you don't address the feedback and concerns raised, they won't be merged. The SPIRV-Cross maintainers and Alyssa have clarified that the current state of these PRs is unacceptable. |
CMakeLists.txt
Outdated
| @@ -0,0 +1,28 @@ | |||
| cmake_minimum_required(VERSION 3.25) | |||
There was a problem hiding this comment.
IMO, the CMake changes should be kept in only #1947, it's out of scope for this PR.
There was a problem hiding this comment.
Yep; fully agree. Thanks for the feedback!
As an open-source project, MoltenVK is dependent on contributions, and sometimes those contributions are driven by client needs that motivate them to contribute. This particular contribution was funded by a customer that had specific platform emulation targets to meet. TF is an incredibly complex and messy project, and a lot of great work has gone into trying to support it with this PR. Thanks for the contribution! This looks to be a creative start!
@alyssarosenzweig makes an excellent point that the main reason for reluctantly bolting TR onto Vulkan was to support its legacy use in tessellation and geometry pipelines. So at the very least, it would help to be clarify what specifically is in scope with this PR. What is included, and what are the client's use cases for the functionality as captured here? There are some tessellation-area changes included here. To what extent does this PR support tessellation? At all? If a specific funding client has a need for only a very small part of TF, and this PR only covers that, then I agree with the argument that advertising general TF support will only frustrate the majority of other users who will be expecting something reasonably close to conformity, and specifically solving the common TF use case of tessellation support. If the client has use for the current limited functionality, what we might look at is hiding advertising the extension behind an environment variable, that the client (and anyone else that can make use of the limited support) can enable to expose that support. And we'll need to include documentation that clearly indicates what those users can expect if they expose the TF extension. Other options could be to leave this PR open as WIP, or to pull it into a branch, for further work. |
|
We're moving this PR back into WIP state to get tess shader support from both popular demand and agreement with customer. |
This is a WIP to provide support to get Transform Feedback into MoltenVk
CMake build capability is an experiment at this time and currently only supports macOS builds.