[Mesa-dev] [PATCH 13/16] i965/fs: Add a saturation propagation optimization pass.

[Matt Turner]

On Thu, Dec 26, 2013 at 5:54 AM, Pohjolainen, Topi
<topi.pohjolainen at intel.com> wrote:
> On Thu, Dec 19, 2013 at 01:40:27PM -0800, Matt Turner wrote:
>> Transforms, for example,
>>
>> mul     vgrf3, vgrf2, vgrf1
>> mov.sat vgrf4, vgrf3
>>
>> into
>>
>> mul.sat vgrf3, vgrf2, vgrf1
>> mov     vgrf4, vgrf3
>>
>> which gives register_coalescing an opportunity to remove the MOV
>> instruction.
>>
>> total instructions in shared programs: 1512588 -> 1501297 (-0.75%)
>> instructions in affected programs:     723097 -> 711806 (-1.56%)
>> GAINED:                                0
>> LOST:                                  4
>> ---
>>  src/mesa/drivers/dri/i965/Makefile.sources         |   1 +
>>  src/mesa/drivers/dri/i965/brw_fs.cpp               |   1 +
>>  src/mesa/drivers/dri/i965/brw_fs.h                 |   1 +
>>  .../dri/i965/brw_fs_saturate_propagation.cpp       | 102 +++++++++++++++++++++
>>  4 files changed, 105 insertions(+)
>>  create mode 100644 src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources
>> index 36d8261..3f901da 100644
>> --- a/src/mesa/drivers/dri/i965/Makefile.sources
>> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
>> @@ -62,6 +62,7 @@ i965_FILES = \
>>       brw_fs_live_variables.cpp \
>>       brw_fs_peephole_predicated_break.cpp \
>>       brw_fs_reg_allocate.cpp \
>> +     brw_fs_saturate_propagation.cpp \
>>       brw_fs_sel_peephole.cpp \
>>       brw_fs_value_numbering.cpp \
>>       brw_fs_vector_splitting.cpp \
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 08837da..12b6d4a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -3304,6 +3304,7 @@ fs_visitor::run()
>>           progress = opt_peephole_sel() || progress;
>>           progress = dead_control_flow_eliminate(this) || progress;
>>           vn_progress = opt_value_numbering();
>> +         progress = opt_saturate_propagation() || progress;
>>           progress = register_coalesce() || progress;
>>        progress = compute_to_mrf() || progress;
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>> index 75de18e..07360ae 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -368,6 +368,7 @@ public:
>>     void try_replace_with_sel();
>>     bool opt_peephole_sel();
>>     bool opt_peephole_predicated_break();
>> +   bool opt_saturate_propagation();
>>     void emit_bool_to_cond_code(ir_rvalue *condition);
>>     void emit_if_gen6(ir_if *ir);
>>     void emit_unspill(fs_inst *inst, fs_reg reg, uint32_t spill_offset,
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp
>> new file mode 100644
>> index 0000000..91a1338
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_saturate_propagation.cpp
>> @@ -0,0 +1,102 @@
>> +/*
>> + * Copyright © 2013 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +#include "brw_fs.h"
>> +#include "brw_fs_live_variables.h"
>> +#include "brw_cfg.h"
>> +
>> +/** @file brw_fs_saturate_propagation.cpp
>> + */
>> +
>> +static bool
>> +opt_saturate_propagation_local(fs_visitor *v, bblock_t *block)
>> +{
>> +   bool progress = false;
>> +   int ip = block->start_ip;
>> +
>> +   for (fs_inst *inst = (fs_inst *)block->start;
>> +        inst != block->end->next;
>> +        inst = (fs_inst *) inst->next) {
>> +      ip++;
>> +
>> +      if (inst->opcode != BRW_OPCODE_MOV ||
>> +          inst->dst.file != GRF ||
>> +          !inst->saturate)
>> +         continue;
>> +
>> +      int src_var = v->live_intervals->var_from_reg(&inst->src[0]);
>> +      int src_end_ip = v->live_intervals->end[src_var];
>> +      if (src_end_ip > ip && !inst->dst.equals(inst->src[0]))
>> +         continue;
>> +
>> +      int scan_ip = ip;
>> +      bool interfered = false;
>> +      for (fs_inst *scan_inst = (fs_inst *) inst->prev;
>> +           scan_inst != block->start->prev;
>> +           scan_inst = (fs_inst *) scan_inst->prev) {
>> +         scan_ip--;
>> +
>> +         if (scan_inst->dst.file == GRF &&
>> +             scan_inst->dst.reg == inst->src[0].reg &&
>> +             scan_inst->dst.reg_offset == inst->src[0].reg_offset &&
>> +             scan_inst->can_do_saturate()) {
>> +            scan_inst->saturate = true;
>
> Now once again trying to educate myself. If I'm reading this correctly it is
> possible (at least in theory) to keep on setting multiple instructions as
> saturated. Below there is the check for interference that controls if the loop
> should be terminated before reaching the beginning of the block.

Right you are. I think I'm missing a break after progress = true. If
we move the saturation modifier that should be the end of the
scan_inst loop.

> Would you have a simple example where an instruction is set saturated here but
> where the 'interfered = true' assignment further down is not taken?

This is really just the example in the commit message, I think. Unless
you mean an example where we move the saturation modifier but continue
searching for other instructions to copy it to -- but adding the
missing break will correctly prevent that.

> Or is it
> simply impossible and the check for interference is separate simply because it
> is taking care of the case where the instruction writing the source of the
> saturated mov-instuction cannot be set as saturated.

Oh, you bring up a good point. What if we find an instruction that
cannot take a saturation modifier writing to our destination? In that
case, we should set consider interfered to be true and break from the
scan_inst loop. I'll change this code to be

         if (scan_inst->dst.file == GRF &&
             scan_inst->dst.reg == inst->src[0].reg &&
             scan_inst->dst.reg_offset == inst->src[0].reg_offset) {
            if (scan_inst->can_do_saturate()) {
               scan_inst->saturate = true;
               inst->saturate = false;
               progress = true;
            }
            break;
         }

Thanks Topi!