[Mesa-dev] [PATCH] i965: Clean up FBH code.

[Matt Turner]

On Fri, Oct 30, 2015 at 5:28 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 10/29/2015 05:52 PM, Matt Turner wrote:
>> It did a bunch of unnecessary stuff, emitting an extra MOV included.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp   |  7 +++----
>>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 18 +++++-------------
>>  2 files changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> index 9c1f95c..b596614 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> @@ -906,12 +906,11 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
>>         * from the LSB side. If FBH didn't return an error (0xFFFFFFFF), then
>>         * subtract the result from 31 to convert the MSB count into an LSB count.
>>         */
>> -
>>        bld.CMP(bld.null_reg_d(), result, fs_reg(-1), BRW_CONDITIONAL_NZ);
>> -      fs_reg neg_result(result);
>> -      neg_result.negate = true;
>> -      inst = bld.ADD(result, neg_result, fs_reg(31));
>> +
>> +      inst = bld.ADD(result, result, fs_reg(31));
>>        inst->predicate = BRW_PREDICATE_NORMAL;
>> +      inst->src[0].negate = true;
>
> This seems like a separate cleanup?  If you choose to split it into a
> separate patch, that patch is

It's just cleaning up the i965/fs code, which is already simpler. I'll
split the i965/fs and vec4 changes into separate patches.

> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
>>        break;
>>     }
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> index 33cc02e..5463f3e 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> @@ -1291,26 +1291,18 @@ vec4_visitor::nir_emit_alu(nir_alu_instr *instr)
>>
>>     case nir_op_ufind_msb:
>>     case nir_op_ifind_msb: {
>> -      src_reg temp = src_reg(this, glsl_type::uint_type);
>> -
>> -      inst = emit(FBH(dst_reg(temp), op[0]));
>> -      inst->dst.writemask = WRITEMASK_XYZW;
>> +      emit(FBH(retype(dst, BRW_REGISTER_TYPE_UD), op[0]));
>>
>>        /* FBH counts from the MSB side, while GLSL's findMSB() wants the count
>>         * from the LSB side. If FBH didn't return an error (0xFFFFFFFF), then
>>         * subtract the result from 31 to convert the MSB count into an LSB count.
>>         */
>> +      src_reg src(dst);
>> +      emit(CMP(dst_null_d(), src, src_reg(-1), BRW_CONDITIONAL_NZ));
>>
>> -      /* FBH only supports UD type for dst, so use a MOV to convert UD to D. */
>> -      temp.swizzle = BRW_SWIZZLE_NOOP;
>> -      emit(MOV(dst, temp));
>> -
>> -      src_reg src_tmp = src_reg(dst);
>> -      emit(CMP(dst_null_d(), src_tmp, src_reg(-1), BRW_CONDITIONAL_NZ));
>> -
>> -      src_tmp.negate = true;
>> -      inst = emit(ADD(dst, src_tmp, src_reg(31)));
>> +      inst = emit(ADD(dst, src, src_reg(31)));
>>        inst->predicate = BRW_PREDICATE_NORMAL;
>> +      inst->src[0].negate = true;
>>        break;
>
> Did retype() not exist when support for ir_unop_find_msb was added?  The
> original code seems... like a weird way to do it.

Right, it was added later.