[Mesa-dev] [PATCH 4/5] i965/vec4_nir: Do boolean source modifier resolves on BDW+

[Jason Ekstrand]

On Mon, Aug 10, 2015 at 11:30 AM, Matt Turner <mattst88 at gmail.com> wrote:
> On Mon, Aug 10, 2015 at 11:19 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Mon, Aug 10, 2015 at 11:16 AM, Matt Turner <mattst88 at gmail.com> wrote:
>>> On Mon, Aug 3, 2015 at 5:22 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>> On BDW+, the negation source modifier on NOT, AND, OR, and XOR, is actually
>>>> a boolean negate and not an integer negate.  However, NIR's soruce
>>>> modifiers are the integer version.  We have to resolve it with a MOV prior
>>>> to emitting the actual instruction.  This is basically the same thing we do
>>>> in the FS backend.
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_vec4.h           |  1 +
>>>>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp     | 15 +++++++++++++++
>>>>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 13 +++++++++++++
>>>>  3 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
>>>> index 985886d..ff143be 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
>>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
>>>> @@ -320,6 +320,7 @@ public:
>>>>                     dst_reg dst, src_reg src0, src_reg src1);
>>>>
>>>>     src_reg fix_3src_operand(src_reg src);
>>>> +   src_reg resolve_source_modifiers(src_reg src);
>>>>
>>>>     vec4_instruction *emit_math(enum opcode opcode, const dst_reg &dst, const src_reg &src0,
>>>>                                 const src_reg &src1 = src_reg());
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>>>> index 27f23d0..1b7fb5e 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>>>> @@ -1020,18 +1020,33 @@ vec4_visitor::nir_emit_alu(nir_alu_instr *instr)
>>>>     }
>>>>
>>>>     case nir_op_inot:
>>>> +      if (devinfo->gen >= 8) {
>>>> +         op[0] = resolve_source_modifiers(op[0]);
>>>> +      }
>>>>        emit(NOT(dst, op[0]));
>>>>        break;
>>>>
>>>>     case nir_op_ixor:
>>>> +      if (devinfo->gen >= 8) {
>>>> +         op[0] = resolve_source_modifiers(op[0]);
>>>> +         op[1] = resolve_source_modifiers(op[1]);
>>>> +      }
>>>>        emit(XOR(dst, op[0], op[1]));
>>>>        break;
>>>>
>>>>     case nir_op_ior:
>>>> +      if (devinfo->gen >= 8) {
>>>> +         op[0] = resolve_source_modifiers(op[0]);
>>>> +         op[1] = resolve_source_modifiers(op[1]);
>>>> +      }
>>>>        emit(OR(dst, op[0], op[1]));
>>>>        break;
>>>>
>>>>     case nir_op_iand:
>>>> +      if (devinfo->gen >= 8) {
>>>> +         op[0] = resolve_source_modifiers(op[0]);
>>>> +         op[1] = resolve_source_modifiers(op[1]);
>>>> +      }
>>>>        emit(AND(dst, op[0], op[1]));
>>>>        break;
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>>> index c5c0d2c..639f829 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>>> @@ -313,6 +313,19 @@ vec4_visitor::fix_3src_operand(src_reg src)
>>>>  }
>>>>
>>>>  src_reg
>>>> +vec4_visitor::resolve_source_modifiers(src_reg src)
>>>> +{
>>>> +   if (!src.abs && !src.negate)
>>>> +      return src;
>>>> +
>>>> +   dst_reg resolved = dst_reg(this, glsl_type::ivec4_type);
>>>> +   resolved.type = src.type;
>>>> +   emit(MOV(resolved, src));
>>>> +
>>>> +   return src_reg(resolved);
>>>> +}
>>>
>>> The FS passes a pointer to an fs_reg and then updates that. Let's do
>>> that here to be consistent.
>>
>> Oh... I was just trying to be consistent with other functions in
>> vec4_visitor such as fix_3src_operand() above.
>
> Heh, consistency is inconsistent!
>
> If changing the FS to be consistent with this seems better, that works for me.
>
> In that case, make the argument 'const src_reg &src' to avoid the copy.

Mind if I do that as a follow-on?  I'll figure out some way to make
these all consistent with each other.