[glsl-in] Validation fixes #898
Conversation
|
For whoever reviews this the commit message for the commits fixing the vector/matrix constructors and the one fixing the parameter qualifier have some more information in them |
JCapucho
force-pushed
the
glsl-fixes
branch
2 times, most recently
from
a05f1ba to
b3bfc23
Compare
| @@ -524,6 +525,26 @@ impl Program<'_> { | |||
| } | |||
| } | |||
|
|
|||
| fn should_read(built_in: BuiltIn, stage: ShaderStage) -> bool { | |||
There was a problem hiding this comment.
This looks like a list that the validator uses. Let the validator take care of this. Here, if you need to exclude specific things, then make it an exclusion list specifically, instead of listing all the possibilities.
There was a problem hiding this comment.
The problem here is that the validator complains if a builtin is added as a function argument in a stage where it shouldn't
There was a problem hiding this comment.
I don't quite understand. If you have an entry point in GLSL that's using some built-in, then just add it to the arguments. If it's not using this built-in, and you aren't adding it to the arguments, then the validator have nothing to complain about.
There was a problem hiding this comment.
You have the case where the built in is being used but shouldn't be written for example (vertex stage)
void main() {
gl_Postition = vec4(1);
}without this fix the backend would produce this pseudocode
vec4 position;
void wrapper(
[builtin=position] vec4 x
) -> [builtin=position] vec4 {
position = x;
main();
return position;
}
void main() {
position = vec4(1);
}this is invalid because position can't be read in the vertex stage, with this change the generated pseudocode would look like this
vec4 position;
void wrapper() -> [builtin=position] vec4 {
main();
return position;
}
void main() {
position = vec4(1);
}There was a problem hiding this comment.
Thank you for writing this down! I think there is a problem in the first step. Why does the pseudocode have "position = x" at all? When GLSL is parsed, we should see that gl_Position (not specifically it, anything goes) is on the left side of an assignment, so there is no reading going on.
There was a problem hiding this comment.
The frontend doesn't keep global use information even though it should because with multiple stages we can't blindly emit globals arguments, I think we should add this for now and I'll make another PR which collects the global use of the functions and the functions it calls recursively and only emit the loads and the stores for the return based on it.
There was a problem hiding this comment.
I see. Ok, in this case, let's add a TODO above this function to follow-up.
|
Thank you for addressing the concerns! I think only "should_read" stuff is left to discuss. |
Matrix and vector constructors expected a vector (in the case of the matrix) or a scalar (in both cases). Now they handle resizing such that the following code now works ```glsl mat3 a; mat4(a); // This would return a validation error because it emitted a cast ```
Previously the types where changed in a later phase now they are changed
right when the argument is added, this makes the implementation slightly
less spec compliant because the following code will compile whilst it shouldn't
```glsl
void test(float a) {}
void test(out float b) {}
```
|
@kvark it should be ready for merging now |
Fixes #884
Fixes #887
Fixes included
VaryingError