[Mesa-dev] [PATCH 15/24] i965: Use LZD to implement nir_op_ufind_msb

[Francisco Jerez]

Ian Romanick <idr at freedesktop.org> writes:

> On 06/30/2016 03:33 PM, Francisco Jerez wrote:
>> Matt Turner <mattst88 at gmail.com> writes:
>> 
>>> On Wed, Jun 29, 2016 at 2:04 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>>
>>>> This uses one less instruction.
>>>
>>> Add FBH to the list of stupid instructions.
>>>
>>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_fs.h               |  4 ++++
>>>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp   |  3 +++
>>>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp         | 26 +++++++++++++++++++++++-
>>>>  src/mesa/drivers/dri/i965/brw_vec4.h             |  4 ++++
>>>>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp |  3 +++
>>>>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp       | 22 ++++++++++++++++++++
>>>>  6 files changed, 61 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>>>> index 4237197..22ce092 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>>>> @@ -237,6 +237,10 @@ public:
>>>>                           nir_tex_instr *instr);
>>>>     void nir_emit_jump(const brw::fs_builder &bld,
>>>>                        nir_jump_instr *instr);
>>>> +   void nir_emit_find_msb_using_lzd(const brw::fs_builder &bld,
>>>> +                                    const fs_reg &result,
>>>> +                                    const fs_reg &src,
>>>> +                                    bool is_signed);
>>>>     fs_reg get_nir_src(const nir_src &src);
>>>>     fs_reg get_nir_src_imm(const nir_src &src);
>>>>     fs_reg get_nir_dest(const nir_dest &dest);
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>>>> index d25d26a..bda4a26 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>>>> @@ -1761,6 +1761,9 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>>>>           /* FBL only supports UD type for dst. */
>>>>           brw_FBL(p, retype(dst, BRW_REGISTER_TYPE_UD), src[0]);
>>>>           break;
>>>> +      case BRW_OPCODE_LZD:
>>>> +         brw_LZD(p, dst, src[0]);
>>>> +         break;
>>>>        case BRW_OPCODE_CBIT:
>>>>           assert(devinfo->gen >= 7);
>>>>           /* CBIT only supports UD type for dst. */
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>>> index b3f5dfd..f15bf3e 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>>> @@ -617,6 +617,25 @@ fs_visitor::optimize_frontfacing_ternary(nir_alu_instr *instr,
>>>>  }
>>>>
>>>>  void
>>>> +fs_visitor::nir_emit_find_msb_using_lzd(const fs_builder &bld,
>>>> +                                        const fs_reg &result,
>>>> +                                        const fs_reg &src,
>>>> +                                        bool is_signed)
>>>> +{
>>>> +   fs_inst *inst;
>>>> +
>>>> +   bld.LZD(retype(result, BRW_REGISTER_TYPE_UD), src);
>>>> +
>>>> +   /* LZD counts from the MSB side, while GLSL's findMSB() wants the count
>>>> +    * from the LSB side. Subtract the result from 31 to convert the MSB
>>>> +    * count into an LSB count.  If no bits are set, LZD will return 32.
>>>> +    * 31-32 = -1, which is exactly what findMSB() is supposed to return.
>>>> +    */
>>>> +   inst = bld.ADD(result, retype(result, BRW_REGISTER_TYPE_D), brw_imm_d(31));
>>>> +   inst->src[0].negate = true;
>>>> +}
>>>
>>> I'd personally be inclined to just inline these functions. I know they
>>> grow somewhat in the next patches... whatever your preference is.
>> 
>> It seems to grow quite a bit in PATCH 16, and it's used in multiple
>> places, right?  How about we keep it factored out but make it a
>> stand-alone function instead of a visitor method?  It doesn't look like
>> it uses *any* internal or external data structures of fs_visitor, it
>> doesn't even dereference the 'this' pointer at all AFAICT, so you could
>> likely improve encapsulation somewhat by making it a static function
>> local to the brw_*_nir.cpp source files.
>
> Okay... two problems:
>
> In the vec4 visitor, all of the emit() functions are in visitor, so this
> new function has to stay there.
>
Ah, right, we should finish the transition to the i965 IR builder in the
vec4 back-end someday.  You could potentially pass a vec4_builder as
argument to the function though, or a vec4_visitor pointer, it's up to
you.

> In the fs visitor, the next patch introduces a call to fs_visitor::vgrf.
>  See previous problem. :(
>
You can use fs_builder::vgrf() instead.

>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160630/baf63c6d/attachment.sig>