[Mesa-dev] [PATCH 03/11] i965: Pull stage_prog_data.nr_params out of the NIR shader
Iago Toral
itoral at igalia.com
Thu Oct 1 08:09:57 PDT 2015
On Thu, 2015-10-01 at 07:58 -0700, Jason Ekstrand wrote:
> On Thu, Oct 1, 2015 at 7:52 AM, Iago Toral <itoral at igalia.com> wrote:
> > On Wed, 2015-09-30 at 18:41 -0700, Jason Ekstrand wrote:
> >> Previously, we had a bunch of code in each stage to figure out how many
> >> slots we needed in stage_prog_data.param. This code was mostly identical
> >> across the stages and had been copied and pasted around. Unfortunately,
> >> this meant that any time you did something special, you had to add code for
> >> it to each of these places. In particular, none of the stages took
> >> subroutines into account; they were working entirely by accident. By
> >> taking this data from the NIR shader, we know the exact number of entries
> >> we need and everything goes a bit smoother.
> >> ---
> >> src/mesa/drivers/dri/i965/brw_cs.c | 4 ++--
> >> src/mesa/drivers/dri/i965/brw_gs.c | 5 ++---
> >> src/mesa/drivers/dri/i965/brw_vs.c | 16 ++++------------
> >> src/mesa/drivers/dri/i965/brw_wm.c | 10 +++-------
> >> 4 files changed, 11 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_cs.c b/src/mesa/drivers/dri/i965/brw_cs.c
> >> index 02eeeda..24120fb 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_cs.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_cs.c
> >> @@ -30,6 +30,7 @@
> >> #include "intel_mipmap_tree.h"
> >> #include "brw_state.h"
> >> #include "intel_batchbuffer.h"
> >> +#include "glsl/nir/nir.h"
> >>
> >> static bool
> >> brw_codegen_cs_prog(struct brw_context *brw,
> >> @@ -55,8 +56,7 @@ brw_codegen_cs_prog(struct brw_context *brw,
> >> * prog_data associated with the compiled program, and which will be freed
> >> * by the state cache.
> >> */
> >> - int param_count = cs->base.num_uniform_components +
> >> - cs->base.NumImages * BRW_IMAGE_PARAM_SIZE;
> >> + int param_count = cp->program.Base.nir->num_uniforms;
> >>
> >> /* The backend also sometimes adds params for texture size. */
> >> param_count += 2 * ctx->Const.Program[MESA_SHADER_COMPUTE].MaxTextureImageUnits;
> >> diff --git a/src/mesa/drivers/dri/i965/brw_gs.c b/src/mesa/drivers/dri/i965/brw_gs.c
> >> index 61e7b2a..0cf7ec8 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_gs.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_gs.c
> >> @@ -32,6 +32,7 @@
> >> #include "brw_vec4_gs_visitor.h"
> >> #include "brw_state.h"
> >> #include "brw_ff_gs.h"
> >> +#include "glsl/nir/nir.h"
> >>
> >>
> >> bool
> >> @@ -60,9 +61,7 @@ brw_codegen_gs_prog(struct brw_context *brw,
> >> * every uniform is a float which gets padded to the size of a vec4.
> >> */
> >> struct gl_shader *gs = prog->_LinkedShaders[MESA_SHADER_GEOMETRY];
> >> - int param_count = gs->num_uniform_components * 4;
> >> -
> >> - param_count += gs->NumImages * BRW_IMAGE_PARAM_SIZE;
> >> + int param_count = gp->program.Base.nir->num_uniforms * 4;
> >
> > I think the vec4 nir backend does not handle image uniforms at the
> > moment, does it? At least I see that the FS backend has code
> > specifically for that in fs_visitor::nir_setup_uniform. Not sure if we
> > support images in geometry stages though, but the code you remove seems
> > to account for that...
>
> We don't. When Curro initially implemented it, he did have vec4
> support. However, that was dropped in favor of doing things
> differently in the fs compiler. It may yet come back though. This
> code was probably merged a bit speculatively but doesn't really hurt
> anything.
Yeah, that's what I recall too, I was surprised to find image stuff here
and thought that maybe I missed something.
> >> c.prog_data.base.base.param =
> >> rzalloc_array(NULL, const gl_constant_value *, param_count);
> >> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
> >> index e1a0d9c..391411c 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_vs.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_vs.c
> >> @@ -109,18 +109,10 @@ brw_codegen_vs_prog(struct brw_context *brw,
> >> * prog_data associated with the compiled program, and which will be freed
> >> * by the state cache.
> >> */
> >> - int param_count;
> >> - if (vs) {
> >> - /* We add padding around uniform values below vec4 size, with the worst
> >> - * case being a float value that gets blown up to a vec4, so be
> >> - * conservative here.
> >> - */
> >> - param_count = vs->base.num_uniform_components * 4 +
> >> - vs->base.NumImages * BRW_IMAGE_PARAM_SIZE;
> >> - stage_prog_data->nr_image_params = vs->base.NumImages;
> >> - } else {
> >> - param_count = vp->program.Base.Parameters->NumParameters * 4;
> >> - }
> >> + int param_count = vp->program.Base.nir->num_uniforms;
> >> + if (!brw->intelScreen->compiler->scalar_vs)
> >> + param_count *= 4;
> >
> > Same thing here.
> >
> > Also, I guess this also means that for the scalar_vs cases we were
> > computing a larger param_count than we really should, right?
>
> They were, but it had nothing to do with images. It only had to do
> with the fact that we were multiplying num_uniform_components by 4
> which means that if your uniforms were all a bunch of floats, it would
> be 4x the size needed.
Yes, I was talking about that too.
Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> >> /* vec4_visitor::setup_uniform_clipplane_values() also uploads user clip
> >> * planes as uniforms.
> >> */
> >> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
> >> index cc97d6a..08f2416 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> >> @@ -35,6 +35,7 @@
> >> #include "program/prog_parameter.h"
> >> #include "program/program.h"
> >> #include "intel_mipmap_tree.h"
> >> +#include "glsl/nir/nir.h"
> >>
> >> #include "util/ralloc.h"
> >>
> >> @@ -173,14 +174,9 @@ brw_codegen_wm_prog(struct brw_context *brw,
> >> * prog_data associated with the compiled program, and which will be freed
> >> * by the state cache.
> >> */
> >> - int param_count;
> >> - if (fs) {
> >> - param_count = fs->base.num_uniform_components +
> >> - fs->base.NumImages * BRW_IMAGE_PARAM_SIZE;
> >> + int param_count = fp->program.Base.nir->num_uniforms;
> >> + if (fs)
> >> prog_data.base.nr_image_params = fs->base.NumImages;
> >> - } else {
> >> - param_count = fp->program.Base.Parameters->NumParameters * 4;
> >> - }
> >> /* The backend also sometimes adds params for texture size. */
> >> param_count += 2 * ctx->Const.Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits;
> >> prog_data.base.param =
> >
> >
>
More information about the mesa-dev
mailing list