[SPIR-V][Sema] Requires nointerpolation on GetAttributeAtVertex param #0

[Keenuts]

This commit adds new sema checks for SPIR-V to restrict the scope of the feature: the nointerpolation attribute is now required on all parameters used in GetAttributeAtVertex, and doesn't propagage magically to parent/child functions.

The current accepted cases this now blocks were mostly broken later in the codegen, so this should be an acceptable 'regression'.

Related to #6220
Fixes #6384, #6383, #6382

[Keenuts]

Hello! Limited the scope of this change to the SPIR-V backend. Added some tests to make sure the DXIL side always catch those as part of the validation.

[Keenuts]

Rebased on main to fix the bots.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[damyanp]

Hello! Limited the scope of this change to the SPIR-V backend. Added some tests to make sure the DXIL side always catch those as part of the validation.

Could you update the PR title and description to reflect the change in scope please?

[damyanp]

Also, can you confirm that with this change scoped to just affect SPIR-V, does it really address #6220?, or should we remove that from the description?

[damyanp]

Can you update the description to clarify if this impacts just SPIR-V or DXIL as well?

If it is just SPIR-V, can you double check the list of issues fixed by this, since one of them is a DXIL issue.

[Keenuts]

Can you update the description to clarify if this impacts just SPIR-V or DXIL as well?

If it is just SPIR-V, can you double check the list of issues fixed by this, since one of them is a DXIL issue.

Right, forgot about the description.
Updated the issue list to only reference the one impacting both DXIL and SPIR-V, and changed title+description to say this only affects SPIR-V.
Thanks!

[tex3d]

I have some concerns with the approach, but realize it is (likely too) late to voice them. Since it's only enabled for SPIR-V, we have less grounds for objection, but we should be clear that this is not likely to be the solution for validating this scenario going forward.

GetAttributeAtVertex issues are in a class shared with EvaluateAttribute* intrinsics. This change can't impact/help those intrinsics for those similar use cases.

At a high-level, I would prefer a warning diagnostic (default error on SPIR-V) that catches any use of this class of intrinsic outside the entry point (and reachable from an active entry point). This would stop catch a lot more invalid use cases that we could not easily diagnose in SemaHLSL. It would hit some cases that work today, but as a warning defaulting to error, it could be disabled for cases that are known to work for someone's specific scenario. The idea is to put the warning up where we know the language can't handle this reliably today, even though it works in some cases today due to some unreliable/unpredictable "magic".

The language implications aren't consistent with HLSL's copy-in parameter passing, or the limited scope where interpolation modifiers have any meaning. I think there are legitimate scenarios this would block, and other illegal ones that would be allowed through this check. I think it would be good to at least make the diagnostic a warning, which defaults to error (and has a unique warning group). This would allow someone to work around a blocked scenario when it doesn't impact GetAttributeAtVertex.

Also, how does this deal with implicitly nointerpolation inputs - any values of non-float types. There's no requirement to use nointerpolation for those.

I can think of scenarios that might pass the diagnostic, and I don't know what the behavior will be for these. I think we could use a few more some test cases for this.

[damyanp]

Thanks for updating the description. It looks like Tex has also added some notes on the code.

[Keenuts]

At a high-level, I would prefer a warning diagnostic (default error on SPIR-V) that catches any use of this class of intrinsic outside the entry point (and reachable from an active entry point). This would stop catch a lot more invalid use cases that we could not easily diagnose in SemaHLSL.

I'd be fine with blocking that too.

I think there are legitimate scenarios this would block, and other illegal ones that would be allowed through this check.

My point of view is: the feature is ill-defined, and cannot be implemented as is.
Knowing that, we tried to implement it. As a result, we have many edge cases we fail to handle: the compiler either crashes, or displays a validation error (which on the the SPIR-V side, is considered a compiler bug).

If I'm building an engine, and use DXC, I'd rather see a feature be disabled with a clear error message, than having to deal with validation errors, compiler instabilities, or guess in which scenario can this feature be used.

But I understand removing the feature is a bit extreme, so what I'm trying is to at least drastically reduce the scope of the feature, limiting the magic required to make it work.

Only allowing the function in the entrypoint would for example go in that direction: remove most of the ill-defined scenarios, to have something predictable and usable.

[sudonatalie]

My point of view is: the feature is ill-defined, and cannot be implemented as is. Knowing that, we tried to implement it. As a result, we have many edge cases we fail to handle: the compiler either crashes, or displays a validation error (which on the the SPIR-V side, is considered a compiler bug).

+1. We've spent a lot of time trying to figure out a reasonable way to match the DXIL behavior in SPIR-V and it's just not feasible with the current semantics. This PR will get the SPIR-V backend to a better state of erroring instead of crashing or throwing unparseable validation errors. If we get to a re-design in microsoft/hlsl-specs#181 then we'll have a good foundation to implement the SPIR-V backend from but I think this is the best option in the meantime.