[Mesa-dev] [PATCH] i965: Implement get_nir_src_imm() helpers.

[Connor Abbott]

On Wed, Dec 2, 2015 at 3:26 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Normally, get_nir_src() won't return an immediate value - it moves it to
> a temporary VGRF.  There are consumers of get_nir_src() that rely on
> this, and are unprepared to handle immediate values.
>
> This patch introduces new helpers which return immediates for single
> component constant values, and VGRFs for other values.

Just curious, what are you going to use this for? Constant propagation
should always be smart enough to fold constants into the instruction,
since the constants are SSA values that turn into registers which are
only written once.

>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs.h         |  1 +
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp   | 11 +++++++++++
>  src/mesa/drivers/dri/i965/brw_vec4.h       |  1 +
>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp |  7 +++++++
>  4 files changed, 20 insertions(+)
>
> I'm using this in my tessellation branch, but I noticed Jason open-coding
> this pattern in a bunch of his recent patches, so I thought I'd send it out
> a bit early in case he wanted to use it, too.
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index bca4589..ce3bdef 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -256,6 +256,7 @@ public:
>     void nir_emit_jump(const brw::fs_builder &bld,
>                        nir_jump_instr *instr);
>     fs_reg get_nir_src(nir_src src);
> +   fs_reg get_nir_src_imm(nir_src src);
>     fs_reg get_nir_dest(nir_dest dest);
>     fs_reg get_nir_image_deref(const nir_deref_var *deref);
>     void emit_percomp(const brw::fs_builder &bld, const fs_inst &inst,
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 9b50e4e..fc71a0f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1136,6 +1136,17 @@ fs_visitor::get_nir_src(nir_src src)
>     return retype(reg, BRW_REGISTER_TYPE_D);
>  }
>
> +/**
> + * Return an IMM for constants; otherwise call get_nir_src() as normal.
> + */
> +fs_reg
> +fs_visitor::get_nir_src_imm(nir_src src)
> +{
> +   nir_const_value *val = nir_src_as_const_value(src);
> +   return val && src.ssa->num_components == 1 ? fs_reg(brw_imm_d(val->i[0]))
> +                                              : get_nir_src(src);
> +}
> +
>  fs_reg
>  fs_visitor::get_nir_dest(nir_dest dest)
>  {
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 25b1139..0150bb9 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -341,6 +341,7 @@ public:
>                         unsigned num_components = 4);
>     src_reg get_nir_src(nir_src src,
>                         unsigned num_components = 4);
> +   src_reg get_nir_src_imm(nir_src src, enum brw_reg_type type);
>
>     virtual dst_reg *make_reg_for_system_value(int location,
>                                                const glsl_type *type) = 0;
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> index 4aed60e..ffd480a 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> @@ -348,6 +348,13 @@ vec4_visitor::get_nir_src(nir_src src, unsigned num_components)
>     return get_nir_src(src, nir_type_int, num_components);
>  }
>
> +src_reg
> +vec4_visitor::get_nir_src_imm(nir_src src, enum brw_reg_type type)
> +{
> +   nir_const_value *val = nir_src_as_const_value(src);
> +   return val ? retype(src_reg(val->u[0]), type) : get_nir_src(src, type, 1);
> +}

Don't you need to check here if the constant value has one component
like you do in FS? Also, this isn't handling VF immediates, where we
can pack something larger than one component into an immediate...
maybe at least worth a comment?

> +
>  void
>  vec4_visitor::nir_emit_load_const(nir_load_const_instr *instr)
>  {
> --
> 2.6.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev