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

[Matt Turner]

On Wed, Dec 2, 2015 at 7:54 AM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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?

I think that's fine. Optimizing with VF immediates is hard, so we have
opt_vector_float() that gathers things into VFs after the main
optimization loop.