[SPIR-V] Normalize inline SPIR-V for builtins

[Keenuts]

This PR requires #7967

Inline SPIR-V provides mutliple attributes with overlapping capabilities:

• vk::builtin
• vk::ext_input_builtin
• vk::ext_output_builtin
• vk::storage_class
• vk::decorate

The issue is that what should be syntactic sugar was not implemented as such: ext_input_builtin should be sugar for decorate + storage_class. This meant codegen has different paths depending on the attributes used, even if they should in the end have the same effect. As expected, those paths have different behaviors, and some are buggy.

This commits adds a new step in sema: attribute normalization. The target idea is to desugar some attributes into a more basic format, but it requires large changes in CG. As of now, this commits normalizes a bit the attributes in sema, but might lift decoration+storage into ext_input_builtin.

Properly doing de-sugar will require additional changes, especially around parameters & builtin through decorations which works only because codegen checks are not complete.

[Keenuts]

I need more context on the purpose of this PR. Does it fix an issue?

The main goal of this PR is to reduce the amount of special cases in the backend caused by inline SPIR-V. We currently have some edge cases when we encounter attributes like ext_builtin_input, ext_storage_class, etc. Goal is when we have functional overlap to normalize those into a single edge case to handle.

vk::binding is currently accepted by DXC in multiple places, but is not handled the same way as vk::ext_decorate(/* DescriptorSet */) + vk::ext_decorate(/* Binding */). The first inline SPIR-V annotation has special checks to correctly disable the default binding+descriptor behavior when the vk::binding attribute is present. The second doesn't,causing the compiler to add the decoration twice.

Divergences like these have caused issue when using ext_decorate and ext_storage_class to declare builtin vs ext_builtin_input/output.

[s-perron]

vk::binding is currently accepted by DXC in multiple places, but is not handled the same way as vk::ext_decorate(/* DescriptorSet /) + vk::ext_decorate(/ Binding */). The first inline SPIR-V annotation has special checks to correctly disable the default binding+descriptor behavior when the vk::binding attribute is present. The second doesn't,causing the compiler to add the decoration twice.

vk::biding is not inline spir-v. It is not suppose to be handled the same way as vk::ext_decorate. As I mentioned, adding the decoration twice is not an incorrect outcome for DXC. The vk::ext_decorate is suppose to be a pass through opaque decoration. It is the user's responsibility to get that right.

It seems like you're goal is to make sure that the people cannot generate invalid SPIR-V when using inline SPIR-V. I don't think that should be our goal. I'm okay generatating an error if was cannot generate spir-v as the user asked. If they have both builtin input and output, then we have can issue an error because the compiler can specify only one storage class for the variable.

[Keenuts]

vk::biding is not inline spir-v.

That's a fair split. I'm fine removing the vk::binding and vk::location normalization from the PR as we could argue those are reserved for "native" HLSL usage, and that if you go through the inline-SPIR-V route, it's full manual.

Then, I'll argue the following:

• if inline SPIR-V is used on something, disable all high-level automatic things like binding/descriptor set allocation.
• allow normalizing inline SPIR-V sugar (ext_input_builtin vs ext_decorate+ext_storage_class)

Would that be OK as guidelines?

It seems like you're goal is to make sure that the people cannot generate invalid SPIR-V when using inline SPIR-V.

Not completely, be increase the quality of life a bit, making some errors a bit friendlier. But I see how mixing inline and vk::binding/vk::location could bea non-welcome and I'm fine drawing a limit between those and real inline SPIR-V.

Follow up question: should we validate in sema that you are not using vk::binding/vk::location with inline SPIR-V then? Because as of today, the restrictions are quite loose.

[s-perron]

if inline SPIR-V is used on something, disable all high-level automatic things like binding/descriptor set allocation.

As a vague statement I don't want to do that. For example, if we add a decoration that says a variable is not-writeable, should that stop the variable from being assigned a set and binding? It would be annoying for a different reason. I think this needs to be on a case-by-case basis.

A guiding question could be:

Without looking into the operands of the inline spir-v attribute or other thing, can we be almost certain that the user will not want the usual behaviour?

For a decoration and assigning a default set and binding, I would say no. For the storage class decoration and the inclusion in a cbuffer, I would say yes. The decisions we make should be reflected in the spec to say how each attribute should be used.

allow normalizing inline SPIR-V sugar (ext_input_builtin vs ext_decorate+ext_storage_class)

I think those are fine.

Follow up question: should we validate in sema that you are not using vk::binding/vk::location with inline SPIR-V then? Because as of today, the restrictions are quite loose.

As before, this is a very general statement. I don't think it is what the user would expect in most cases. Inline SPIR-V is a "power user" syntax that people should use at their own risk. I don't mind it being flexible. We want it to be flexible so that it can be used for things we do not anticipate.