On Thu, Dec 26, 2013 at 5:54 AM, Pohjolainen, Topi
<topi.pohjolai...@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!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
