[Mesa-dev] [PATCH 34/38] i965/fs: Migrate translation of NIR texturing instructions to the IR builder.

[Matt Turner]

On Mon, Jun 8, 2015 at 7:20 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Matt Turner <mattst88 at gmail.com> writes:
>
>> On Thu, Jun 4, 2015 at 9:05 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs.h       |  3 ++-
>>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 11 ++++-------
>>>  2 files changed, 6 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>>> index 338c816..ef0256d 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>>> @@ -319,7 +319,8 @@ public:
>>>     void nir_emit_alu(const brw::fs_builder &bld, nir_alu_instr *instr);
>>>     void nir_emit_intrinsic(const brw::fs_builder &bld,
>>>                             nir_intrinsic_instr *instr);
>>> -   void nir_emit_texture(nir_tex_instr *instr);
>>> +   void nir_emit_texture(const brw::fs_builder &bld,
>>> +                         nir_tex_instr *instr);
>>>     void nir_emit_jump(const brw::fs_builder &bld,
>>>                        nir_jump_instr *instr);
>>>     fs_reg get_nir_src(nir_src src);
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> index ff5ac9c..61058b2 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> @@ -444,7 +444,6 @@ void
>>>  fs_visitor::nir_emit_instr(nir_instr *instr)
>>>  {
>>>     const fs_builder abld = bld.annotate(NULL, instr);
>>> -   this->base_ir = instr;
>>
>> I'd make this...
>>
>>>
>>>     switch (instr->type) {
>>>     case nir_instr_type_alu:
>>> @@ -456,7 +455,7 @@ fs_visitor::nir_emit_instr(nir_instr *instr)
>>>        break;
>>>
>>>     case nir_instr_type_tex:
>>> -      nir_emit_texture(nir_instr_as_tex(instr));
>>> +      nir_emit_texture(abld, nir_instr_as_tex(instr));
>>>        break;
>>>
>>>     case nir_instr_type_load_const:
>>> @@ -472,8 +471,6 @@ fs_visitor::nir_emit_instr(nir_instr *instr)
>>>     default:
>>>        unreachable("unknown instruction type");
>>>     }
>>> -
>>> -   this->base_ir = NULL;
>>
>> ... and this a separate commit. Otherwise there's an implicit ordering
>> to the patches to brw_fs_nir.cpp.
>
> There is also a similar ordering dependency between PATCH 31 and the
> following because of the "abld" local defined in that file.  Should I
> also split that definition into a separate commit?  It seems silly to
> split a one-line change defining a local variable from the first commit
> that uses it, and a two-line change removing a dead assignment from the
> commit that makes the assignment dead -- Unless you have some reason to
> land these patches in a different order?

Removing the this->base_ir assignments is a clean up enabled by -- but
not part of -- the main part of the patch.

Why don't you just move these hunks to

> i965/fs: Remove dead IR construction code from the visitor.

where you're removing the base_ir field itself and two other assignments to it?

With that,

Reviewed-by: Matt Turner <mattst88 at gmail.com>