MSL: Workaround Metal 3.1 regression bug on recursive input structs.#2217
MSL: Workaround Metal 3.1 regression bug on recursive input structs.#2217HansKristian-Work merged 1 commit intoKhronosGroup:mainfrom billhollings:metal-3-1-regression-fix
Conversation
cdavis5e
left a comment
cdavis5e
left a comment
There was a problem hiding this comment.
LGTM. I have one small comment, though. And I'd probably wait for H-K to give his OK before merging.
|
Deadline for the next SDK has already passed, btw. |
|
If MoltenVK has a different SDK deadline from SPIRV-Cross and it can reference a different SPIRV-Cross commit locally, it would make sense to just push up a frozen branch that MVK can reference in the short-term. SPIRV-Cross upstream shouldn't need to block what ships in MVK SDKs. |
|
If I can get a new spirv cross commit on Monday morning we can use that for the sdk |
Metal 3.1 introduced a Metal regression bug which causes an infinite recursion crash during Metal's analysis of an entry point input structure that itself contains internal recursion. This patch works around this by replacing the recursive input declaration with a alternate variable of type void*, and then casting to the correct type at the top of the entry point function. - Add CompilerMSL::Options::replace_recursive_inputs to enable replacing recursive input. - Add Compiler::type_contains_recursion() to determine if a struct contains internal recursion, and add custom Decorations to mark such structs, to short-cut future similar checks. - Replace recursive input struct declarations with void*, and emit a recast to correct type at top of entry function. - Add unit test. - Compiler::type_is_top_level_block() remove hardcode reference to spirv_cross namespace, as it interferes with configurable namespaces (unrelated).
All good points. I was unaware that SPIRV-Cross had an SDK delivery date at all. MoltenVK typically uses the SPIRV-Cross main HEAD at time of MoltenVK SDK release, and this has worked so far. I guess having a MoltenVK branch on SPIRV-Cross would give us options when something like this comes up, without rushing the main branch. However, this situation has proven extremely rare so far, and maintaining a separate MoltenVK branch feels like it could potentially become a maintenance nightmare for all of us down the road, if it deviates far from the main branch, by accumulating updates that don't quickly make it into the main branch. If we can fit this into Either way, we can have further discussions about whether a dual-branch is needed, and how it can be made effective. And, regardless of future activity, I do appreciate that this situation has proven awkward this time, and I appreciate the rapid review and discussion. |
4 participants
Metal 3.1 introduced a Metal regression bug which causes an infinite recursion crash during Metal's analysis of an entry point input structure that itself contains internal recursion. This patch works around this by replacing the recursive input declaration with a alternate variable of type void*, and then casting to the correct type at the top of the entry point function.
CompilerMSL::Options::replace_recursive_inputsto enable replacing recursive input.Compiler::type_contains_recursion()to determine if a struct contains internal recursion, and add custom Decorations to mark such structs, to short-cut future similar checks.Compiler::type_is_top_level_block()remove hardcode reference to spirv_cross namespace, as it interferes with configurable namespaces (unrelated).@HansKristian-Work This Metal regression surfaced quite recently, as macOS Sonoma was rolled out. This is a fairly simple patch fix that, if at all possible, it would be great to get this pulled in by Mon Oct 16, so it can be included in the upcoming Vulkan SDK release.