Skip to content
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

MSL: Remove special case for threadgroup array wrapper. #2240

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

HansKristian-Work
Copy link
Contributor

@HansKristian-Work HansKristian-Work commented Dec 5, 2023

I cannot find any evidence that this does not actually work. The original case here was from Epic's PR series in 2019, but I cannot see why it doesn't work. It might have been a bug in a very old compiler at some point.

Fix #2239.

@HansKristian-Work
Copy link
Contributor Author

@cdavis5e @LukasBanana Please check if this PR breaks anything for you.

@LukasBanana
Copy link
Contributor

I don't see a problem with this PR since it's handling a case where a wrapper is not allowed. I haven't seen this wrapper in a while but also haven't been working with the Metal backend in our shader pipeline for quite some time. iirc. the idea was to workaround the issue of copying arrays locally but I don't remember the case where this happened off the top of my head. I wouldn't mind if we can get rid of it entirely, but for now, this change should be good.

@HansKristian-Work
Copy link
Contributor Author

HansKristian-Work commented Dec 6, 2023

it's handling a case where a wrapper is not allowed

Not sure what you mean, I'm considering that more cases are allowed here.

The original commit which made this claim was:

commit d50659af92fc379ba33e26cf5c39d4233396cb38
Author: Mark Satterthwaite <[email protected]>
Date:   Tue Aug 13 18:18:48 2019 -0400

    Rework the way arrays are handled in Metal to remove the array copies as they are unnecessary from Metal 1.2. There were cases where copies were not being inserted and others appeared unncessary, using the template type should allow the 'metal' compiler to do the best possible optimisation. The changes are broken into three stages.

1. Allow Metal to use the array<T> template to make arrays a value type.
2. Force the use of C style array declaration for some cases which cannot be wrapped with a template.
3. Threadgroup arrays can't have a wrapper type.
4. Tweak the code to use unsafe_array in a few more places so that we can handle passing arrays of resources into the shader and then through shaders into sub-functions.
5. Handle packed matrix types inside arrays within structs.
6. Make sure that builtin arguments still retain their array qualifiers when used in leaf functions.
7. Fix declaration of array-of-array constants for Metal so we can use the array<T> template.

3. Threadgroup arrays can't have a wrapper type. is wrong I think and is causing me immense pain at the moment since there are various bugs that are unreasonably difficult to fix.

I cannot find any evidence that this does not actually work.
The original case here was from Epic's PR series in 2019, but I cannot see why it doesn't work.
It might have been a bug in a very old compiler at some point.
@HansKristian-Work
Copy link
Contributor Author

Gonna go ahead and merge this. If this somehow breaks, it can always be reverted.

@HansKristian-Work HansKristian-Work merged commit f349c91 into main Dec 7, 2023
6 checks passed
@HansKristian-Work HansKristian-Work deleted the fix-2239 branch December 7, 2023 13:50
@LukasBanana
Copy link
Contributor

Sorry for the late response.

Not sure what you mean, I'm considering that more cases are allowed here.

I meant to say you were probably right, but didn't formulate that well. Perhaps threadgroup arrays were more problematic in earlier versions of Metal. UE5 minimum Metal language support is 2.2 for macOS and 2.4 for iOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MSL] Invalid MSL generated when copying from groupshared array to local
2 participants