spirv-val: Add Vulkan decoration interface #4831
Conversation
|
This triggers in angle_unittests when I try to roll into Chromium |
|
This is due to an ANGLE bug, we didn't emit |
| if (decoration == SpvDecorationFlat || | ||
| decoration == SpvDecorationNoPerspective || | ||
| decoration == SpvDecorationSample || | ||
| decoration == SpvDecorationCentroid) { | ||
| // VUID 04670 already validates these decorations are input/output | ||
| if (storage_class == SpvStorageClassInput && | ||
| (models->size() > 1 || has_vert)) { | ||
| return vstate.diag(SPV_ERROR_INVALID_ID, var_instr) | ||
| << vstate.VkErrorID(6202) | ||
| << "OpEntryPoint interfaces variable must not be vertex " | ||
| "execution model with an input storage class for " | ||
| "Entry Point id " | ||
| << entry_point << "."; | ||
| } else if (storage_class == SpvStorageClassOutput && | ||
| (models->size() > 1 || has_frag)) { | ||
| return vstate.diag(SPV_ERROR_INVALID_ID, var_instr) | ||
| << vstate.VkErrorID(6201) | ||
| << "OpEntryPoint interfaces variable must not be " | ||
| "fragment " | ||
| "execution model with an output storage class for " | ||
| "Entry Point id " | ||
| << entry_point << "."; |
There was a problem hiding this comment.
I don't understand the phrasing used here:
- "must not be {vertex,fragment} execution model"
- how can a variable be an execution model?
(is there a missing "for" in the middle there?)
- how can a variable be an execution model?
- it (effectively) claims vertex shaders can't have inputs and fragment shaders can't have outputs?!
- without looking up e.g.
VUID-StandaloneSpirv-Flat-06201in the spec there is no way at all to tell that the error is talking about decorations on interface variables, not their storage classes
(let alone which decorations, i.e.Flat/NoPerspective/Sample/Centroid)
- without looking up e.g.
It almost looks like there's a place left in the message for printing the decoration name, but that was forgotten? (e.g. after "must not be" one could add "decorated with Flat", though the rest of the message needs a bit of tweaking too)
There was a problem hiding this comment.
Looking at the PR this replaces, its version of the message uses LogStringForDecoration and overall seems readable, unlike this PR.
Despite the GH username change, it looks like both PRs are by @sjfricke so I don't understand how the message regression happened.
There was a problem hiding this comment.
So I agree the error message is a little vague, can improve that! (unless you were planning on making the PR)
Sorry, I forgot about LogStringForDecoration ... I made both these PR a year apart and there were some spec changes in between that time too that was blocking the original PR.
the successor of #4293 and closes #3154 and #3066
This adds the following: