On Tue, Sep 4, 2018 at 8:15 AM Alejandro Piñeiro <[email protected]>
wrote:
> brw_link_shader is also the responsible to create the NIR. In the case
> of GLSL it uses the IR (process ir + glsl_to_nir), in the case of
> ARB_gl_spirv it uses the spir-v shader (some process + spirv_to_nir).
>
> After getting the NIR, it applies some lowerings.
>
> Until now, ARB_gl_spirv implementation was assuming that those
> lowerings were "pure-NIR", so it started the linking based on NIR
> after brw_create_nir.
>
> But since commit "anv,i965: Lower away image derefs in the driver"
> brw_create_nir calls brw_nir_lower_glsl_images that assumes that the
> UniformStorage was already initialized, so that the uniform linking
> was already done, in order to know the sampler idx.
>
> This pending-to-be cleaned patch just moves the call of this
> lowering. Now it is called after gl_nir_link_uniforms. As far as we
> see, that fixed all regressions on ARB_gl_spirv tests.
>
> But:
>
> * Perhaps it is not really correct to do that. After all,
> gl_nir_link_uniforms links based on the nir shader, and the
> correct would be do the linking after that lowering. In that case
> we would need to compute the sampler_idx in an alternative way.
>
In general, linking should probably happen before lowering anyway. I
really wasn't thinking about linking when I put it there; it was convenient
since it seemed to correspond nicely to brw_nir_lower_uniforms. The only
important thing is that it should really come *after*
brw_nir_lower_uniforms.
> * If it is ok to move the lowering for the ARB_gl_spirv case,
> perhaps it is ok to move it on the GLSL case. In that case the
> solution will be more simple and clean.
>
I see no reason why we can't. Prior to the mentioned commit, we were doing
it in the back-end. I see no reason it can't be lowered later than it is
now.
> Opinions?
>
> Finally, the lowering is called brw_nir_lower_glsl_images. But looking
> a little to it, except for the uniform storage thing, I don't see any
> reason to not use it on the ARB_gl_spirv codepath. So perhaps it would
> be worthy to rename it to brw_nir_lower_opengl_images or
> brw_nir_lower_gl_images?
>
That's fine with me. I just named it that way to separate it from Vulkan;
I wasn't reallythinking about GL SPIR-V.
> ---
> src/mesa/drivers/dri/i965/brw_link.cpp | 11 +++++++++++
> src/mesa/drivers/dri/i965/brw_program.c | 8 ++++++--
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
> b/src/mesa/drivers/dri/i965/brw_link.cpp
> index 0723255dec6..70375ca3f29 100644
> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> @@ -268,6 +268,17 @@ brw_link_shader(struct gl_context *ctx, struct
> gl_shader_program *shProg)
>
> gl_nir_link_assign_atomic_counter_resources(ctx, shProg);
> gl_nir_link_assign_xfb_resources(ctx, shProg);
> +
> + for (unsigned stage = 0; stage <
> ARRAY_SIZE(shProg->_LinkedShaders); stage++) {
> + struct gl_linked_shader *shader = shProg->_LinkedShaders[stage];
> + if (!shader)
> + continue;
> +
> + struct gl_program *prog = shader->Program;
> + prog->Parameters = _mesa_new_parameter_list();
> +
> + NIR_PASS_V(prog->nir, brw_nir_lower_glsl_images, prog);
> + }
> }
>
> /* Determine first and last stage. */
> diff --git a/src/mesa/drivers/dri/i965/brw_program.c
> b/src/mesa/drivers/dri/i965/brw_program.c
> index 041395ec4c0..68f6e18bb09 100644
> --- a/src/mesa/drivers/dri/i965/brw_program.c
> +++ b/src/mesa/drivers/dri/i965/brw_program.c
> @@ -139,8 +139,12 @@ brw_create_nir(struct brw_context *brw,
> }
> }
>
> - NIR_PASS_V(nir, brw_nir_lower_uniforms, is_scalar);
> - NIR_PASS_V(nir, brw_nir_lower_glsl_images, prog);
> + if (shader_prog && shader_prog->data->spirv) {
> + NIR_PASS_V(nir, brw_nir_lower_uniforms, is_scalar);
> + } else {
> + NIR_PASS_V(nir, brw_nir_lower_uniforms, is_scalar);
> + NIR_PASS_V(nir, brw_nir_lower_glsl_images, prog);
> + }
>
> return nir;
> }
> --
> 2.14.1
>
>
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
