KhronosGroup / SPIRV-Cross

SPIRV-Cross is a practical tool and library for performing reflection on SPIR-V and disassembling SPIR-V back to high level languages.
Apache License 2.0
1.96k stars 549 forks source link

MSL: Add support for SPV_EXT_integer_dot_product #2257

Closed spnda closed 5 months ago

spnda commented 5 months ago

As a dot product requires a reduce_add function I added that with overloads for all integer vector types. I don't quite like it, but there's no way around that using templates or something like __builtin_reduce_add which would usually be available with Clang but is not accepted by the Metal compiler.

I think this should cover all of the extra cases, the type widening to match the return type and the 32-bit integer unpacking.

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

billhollings commented 5 months ago

In addition to the reduce_add() template function, is there any value in adding something like a dot() template function for integer vectors, so that the resulting MSL code looks both like the equivalent Metal float vector functions, and like the equivalent GLSL output, and is therefore more readable?

spnda commented 5 months ago

In addition to the reduce_add() template function, is there any value in adding something like a dot() template function for integer vectors, so that the resulting MSL code looks both like the equivalent Metal float vector functions, and like the equivalent GLSL output, and is therefore more readable?

based on a basic version of the function, perhaps. Now that the function is getting a bit more complex, especially around the signed/unsigned conversion and the unpacking of integers, I'm not sure I fully see the gain. Most of the logic that exists for the SPIR-V opcodes to be implemented correctly would have to happen outside of the function due to it having to know more about the context.

Mostly, I am referencing what @HansKristian-Work noticed about the unsigned -> signed integer conversion. I'll have to use codegen to effectively add a as_type<int4>(uint4(x)) when an unsigned integer is provided for a signed integer parameter. I don't think that'd really be possible for a generic function. But this still applies for unpacking too, when the generated code will have to do as_type<uchar4>(x) and whether it casts to char4 or uchar4 is depending on the context, again.

Still, sure, I could add something simple like this:

template <typename T>
T sdot(vec<T, N> vec1, vec<T, N> vec2) {
    return reduce_add(vec1 * vec2);
}

but that would cover a single case. Using functions would require us to write many different generic functions to handle the many different scenarios given. So, apart from added code size and more templates for the Metal compiler to go through, what does this gain over the inline implementation? I don't think it's a good idea.

billhollings commented 5 months ago

I don't think it's a good idea

Fair enough. Yeah...I posed it as a question, because I knew it would be a bit messy given the templates and overloads already in play. It would make the MSL code more understandable to read, but I agree may come at a cost of a lot of extra shader declaration content, and complexity in the generation code within SPIRV-Cross. So...it's a nice-to-have but certainly not worth too much trouble.

HansKristian-Work commented 5 months ago

Merged this locally. Fixing misc issues locally while adding more test coverage as I go.

HansKristian-Work commented 5 months ago

On master.