[Mesa-dev] [PATCH 31/38] i965/fs: Migrate translation of NIR control flow to the IR builder.

[Matt Turner]

On Mon, Jun 8, 2015 at 7:00 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 | 23 ++++++++++++-----------
>>>  2 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>>> index 3f775d3..8803d5a 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(nir_alu_instr *instr);
>>>     void nir_emit_intrinsic(nir_intrinsic_instr *instr);
>>>     void nir_emit_texture(nir_tex_instr *instr);
>>> -   void nir_emit_jump(nir_jump_instr *instr);
>>> +   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_dest(nir_dest dest);
>>>     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 3f84830..f31829f 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> @@ -397,21 +397,21 @@ void
>>>  fs_visitor::nir_emit_if(nir_if *if_stmt)
>>>  {
>>>     /* first, put the condition into f0 */
>>> -   fs_inst *inst = emit(MOV(reg_null_d,
>>> +   fs_inst *inst = bld.MOV(bld.null_reg_d(),
>>>                              retype(get_nir_src(if_stmt->condition),
>>> -                                   BRW_REGISTER_TYPE_D)));
>>> +                                   BRW_REGISTER_TYPE_D));
>>>     inst->conditional_mod = BRW_CONDITIONAL_NZ;
>>>
>>> -   emit(IF(BRW_PREDICATE_NORMAL));
>>> +   bld.IF(BRW_PREDICATE_NORMAL);
>>>
>>>     nir_emit_cf_list(&if_stmt->then_list);
>>>
>>>     /* note: if the else is empty, dead CF elimination will remove it */
>>> -   emit(BRW_OPCODE_ELSE);
>>> +   bld.emit(BRW_OPCODE_ELSE);
>>>
>>>     nir_emit_cf_list(&if_stmt->else_list);
>>>
>>> -   emit(BRW_OPCODE_ENDIF);
>>> +   bld.emit(BRW_OPCODE_ENDIF);
>>>
>>>     if (!try_replace_with_sel() && devinfo->gen < 6) {
>>>        no16("Can't support (non-uniform) control flow on SIMD16\n");
>>> @@ -425,11 +425,11 @@ fs_visitor::nir_emit_loop(nir_loop *loop)
>>>        no16("Can't support (non-uniform) control flow on SIMD16\n");
>>>     }
>>>
>>> -   emit(BRW_OPCODE_DO);
>>> +   bld.emit(BRW_OPCODE_DO);
>>>
>>>     nir_emit_cf_list(&loop->body);
>>>
>>> -   emit(BRW_OPCODE_WHILE);
>>> +   bld.emit(BRW_OPCODE_WHILE);
>>>  }
>>>
>>>  void
>>> @@ -443,6 +443,7 @@ fs_visitor::nir_emit_block(nir_block *block)
>>>  void
>>>  fs_visitor::nir_emit_instr(nir_instr *instr)
>>>  {
>>> +   const fs_builder abld = bld.annotate(NULL, instr);
>>>     this->base_ir = instr;
>>>
>>>     switch (instr->type) {
>>> @@ -465,7 +466,7 @@ fs_visitor::nir_emit_instr(nir_instr *instr)
>>>        break;
>>>
>>>     case nir_instr_type_jump:
>>> -      nir_emit_jump(nir_instr_as_jump(instr));
>>> +      nir_emit_jump(abld, nir_instr_as_jump(instr));
>>
>> Why do we need to change the annotation to NULL?
>
> The annotation string should already have been NULL because the
> translation pass from NIR didn't ever set it to anything else -- And if
> in some case it wasn't NULL, it would have been an annotation previously
> set by some fs_visitor helper method during the translation of some
> previous instruction "leaking" into the current instruction, IOW a bug.

I can't convince myself that it's needed, but okay I guess.

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