Re: [PATCH 3/8] wined3d: Simplify shader interface matching for SM4+ vertex shaders.

[Henri Verbeet]

On 19 April 2016 at 18:56, Matteo Bruni  wrote:
> After some testing it looks like, with SM4+ on Windows:
> - Output registers have to match with input registers from the
>
> following shader stage (register index and mask). There is no "smart"
> matching of semantics like in SM3.
> 2. Using uninitialized varyings in SM4+ shaders doesn't give consistent
> results.
> That means we can simplify the relevant code quite a bit. Applications
> shouldn't be able to depend on particular values for uninitialized
> varyings so we can avoid explicitly initializing them.

Does that mean we don't need the separate "reorder" shader between the
stages? (Even more so if we have separate shader objects.)

On 19 April 2016 at 18:56, Matteo Bruni  wrote:
> -static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv,
> +static void shader_glsl_setup_vs3_output(struct shader_glsl_priv *priv,
>          const struct wined3d_gl_info *gl_info, const DWORD *map,
>          const struct wined3d_shader_signature *input_signature,
>          const struct wined3d_shader_reg_maps *reg_maps_in,
>          const struct wined3d_shader_signature *output_signature,
> -
>    const struct wined3d_shader_reg_maps *reg_maps_out, const char *out_array_name)
> -
>    const struct wined3d_shader_reg_maps *reg_maps_out)
>
> {
>      struct wined3d_string_buffer *destination = string_buffer_get(&priv->string_buffers);
>      BOOL legacy_context = gl_info->supported[WINED3D_GL_LEGACY_CONTEXT];
> @@ -5191,7 +5191,7 @@ static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv,
>          else if (in_idx == in_count + 1)
>              string_buffer_sprintf(destination, "gl_FrontSecondaryColor");
>          else
> -
>        string_buffer_sprintf(destination, "%s[%u]", out_array_name, in_idx);
> -
>        string_buffer_sprintf(destination, "%s[%u]", "ps_link", in_idx);
>
>    if (!set[in_idx])
>        set[in_idx] = ~0u;
>
> @@ -5243,7 +5243,7 @@ static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv,
>          else if (i == in_count + 1)
>              string_buffer_sprintf(destination, "gl_FrontSecondaryColor");
>          else
> -
>        string_buffer_sprintf(destination, "%s[%u]", out_array_name, i);
> -
>        string_buffer_sprintf(destination, "%s[%u]", "ps_link", i);
>
>    if (size == 1)
>        shader_addline(buffer, "%s.%s = 0.0;\n", destination->buffer, reg_mask);

This looks like you should have just kept it the way it was in the
previous patch.

[PATCH 3/8] wined3d: Simplify shader interface matching for SM4+ vertex shaders.

[Matteo Bruni]

2016-04-20 12:50 GMT+02:00 Henri Verbeet :
> On 19 April 2016 at 18:56, Matteo Bruni  wrote:
> > After some testing it looks like, with SM4+ on Windows:
> > - Output registers have to match with input registers from the
> >
> > following shader stage (register index and mask). There is no "smart"
> > matching of semantics like in SM3.
> > 2. Using uninitialized varyings in SM4+ shaders doesn't give consistent
> > results.
> > That means we can simplify the relevant code quite a bit. Applications
> > shouldn't be able to depend on particular values for uninitialized
> > varyings so we can avoid explicitly initializing them.
>
> Does that mean we don't need the separate "reorder" shader between the
> stages? (Even more so if we have separate shader objects.)

We shouldn't really need separate "reorder" shaders for SM4+, correct.
At the moment I'm still making use of the separate shader for the
setup_vs_output() function to be able to use the same VS variant both
with and without a GS. Of course that means right now we need to
compile the "setup" shader in that case which is arguably worse than a
shader variant.
I had in mind to look into that at some point after this first "pass"
at fixing shaders matching but maybe that's not a good plan. I guess
I'll have a shot at dropping the separate shader for SM4 and see what
happens...
> On 19 April 2016 at 18:56, Matteo Bruni  wrote:
> > -static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv,
> > +static void shader_glsl_setup_vs3_output(struct shader_glsl_priv *priv,
> >          const struct wined3d_gl_info *gl_info, const DWORD *map,
> >          const struct wined3d_shader_signature *input_signature,
> >          const struct wined3d_shader_reg_maps *reg_maps_in,
> >          const struct wined3d_shader_signature *output_signature,
> > -
> >    const struct wined3d_shader_reg_maps *reg_maps_out, const char *out_array_name)
> > -
> >    const struct wined3d_shader_reg_maps *reg_maps_out)
> >
> > {
> >      struct wined3d_string_buffer *destination = string_buffer_get(&priv->string_buffers);
> >      BOOL legacy_context = gl_info->supported[WINED3D_GL_LEGACY_CONTEXT];
> > @@ -5191,7 +5191,7 @@ static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv,
> >          else if (in_idx == in_count + 1)
> >              string_buffer_sprintf(destination, "gl_FrontSecondaryColor");
> >          else
> > -
> >        string_buffer_sprintf(destination, "%s[%u]", out_array_name, in_idx);
> > -
> >        string_buffer_sprintf(destination, "%s[%u]", "ps_link", in_idx);
> >
> >    if (!set[in_idx])
> >        set[in_idx] = ~0u;
> >
> > @@ -5243,7 +5243,7 @@ static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv,
> >          else if (i == in_count + 1)
> >              string_buffer_sprintf(destination, "gl_FrontSecondaryColor");
> >          else
> > -
> >        string_buffer_sprintf(destination, "%s[%u]", out_array_name, i);
> > -
> >        string_buffer_sprintf(destination, "%s[%u]", "ps_link", i);
> >
> >    if (size == 1)
> >        shader_addline(buffer, "%s.%s = 0.0;\n", destination->buffer, reg_mask);
>
> This looks like you should have just kept it the way it was in the
> previous patch.

Sure, I can drop that change. Actually it was like that until a couple
of days ago (I reread these patches way too many times...)