On Wed, Oct 30, 2013 at 10:06 AM, Paul Berry <stereotype...@gmail.com> wrote:
> On 28 October 2013 11:31, Matt Turner <matts...@gmail.com> wrote:
>>
>> fs_visitor::try_replace_with_sel optimizes only if statements whose
>> "then" and "else" bodies contain a single MOV instruction. It also did
>> could not handle constant arguments, since they cause an extra MOV
>> immediate to be generated (since we haven't run constant propagation,
>> there are more than the single MOV).
>>
>> This peephole fixes both of these and operates as a normal optimization
>> pass.
>>
>> fs_visitor::try_replace_with_sel is still arguably necessary, since it
>> runs before pull constant loads are lowered.
>>
>> total instructions in shared programs: 1547180 -> 1545254 (-0.12%)
>> instructions in affected programs: 96585 -> 94659 (-1.99%)
>> ---
>> 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 +
>> src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp | 245
>> ++++++++++++++++++++++
>> 4 files changed, 248 insertions(+)
>> create mode 100644 src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
>> b/src/mesa/drivers/dri/i965/Makefile.sources
>> index c4d689e..5ddb421 100644
>> --- a/src/mesa/drivers/dri/i965/Makefile.sources
>> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
>> @@ -59,6 +59,7 @@ i965_FILES = \
>> brw_fs_fp.cpp \
>> brw_fs_generator.cpp \
>> brw_fs_live_variables.cpp \
>> + brw_fs_sel_peephole.cpp \
>> brw_fs_reg_allocate.cpp \
>> brw_fs_vector_splitting.cpp \
>> brw_fs_visitor.cpp \
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 28d369a..d3d2e44 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -3131,6 +3131,7 @@ fs_visitor::run()
>> progress = opt_algebraic() || progress;
>> progress = opt_cse() || progress;
>> progress = opt_copy_propagate() || progress;
>> + progress = opt_peephole_sel() || progress;
>> progress = dead_code_eliminate() || progress;
>> progress = dead_code_eliminate_local() || progress;
>> progress = register_coalesce() || progress;
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
>> b/src/mesa/drivers/dri/i965/brw_fs.h
>> index dff6ec1..a67ef86 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -361,6 +361,7 @@ public:
>> bool try_emit_saturate(ir_expression *ir);
>> bool try_emit_mad(ir_expression *ir, int mul_arg);
>> void try_replace_with_sel();
>> + bool opt_peephole_sel();
>> 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_sel_peephole.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
>> new file mode 100644
>> index 0000000..11c3677
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
>> @@ -0,0 +1,245 @@
>> +/*
>> + * 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_cfg.h"
>> +
>> +/** @file brw_fs_sel_peephole.cpp
>> + *
>> + * This file contains the opt_peephole_sel() optimization pass that
>> replaces
>> + * MOV instructions to the same destination in the "then" and "else"
>> bodies of
>> + * an if statement with SEL instructions.
>> + */
>> +
>> +#define MAX_MOVS 8 /**< The maximum number of MOVs to attempt to match.
>> */
>> +
>> +/**
>> + * Scans backwards from an ENDIF counting MOV instructions with common
>> + * destinations inside the "then" and "else" blocks of the if statement.
>> + *
>> + * A pointer to the fs_inst* for ENDIF is passed as the <match> argument.
>> The
>> + * function stores pointers to the MOV instructions in the <then_mov> and
>> + * <else_mov> arrays. If the function is successful, the <match> points
>> to the
>> + * fs_inst* pointing to the IF instruction at the beginning of the block.
>> + *
>> + * \return the number of MOVs to a common destination found in the two
>> branches
>> + * or zero if an error occurred.
>> + *
>> + * E.g.:
>> + * match = IF ...
>> + * then_mov[1] = MOV g4, ...
>> + * then_mov[0] = MOV g5, ...
>> + * ELSE ...
>> + * else_mov[1] = MOV g4, ...
>> + * else_mov[0] = MOV g5, ...
>> + * ENDIF
>> + * returns 2.
>> + */
>> +static int
>> +match_movs_from_endif(fs_inst *then_mov[MAX_MOVS], fs_inst
>> *else_mov[MAX_MOVS],
>> + fs_inst **match)
>> +{
>> + fs_inst *m = *match;
>> +
>> + assert(m->opcode == BRW_OPCODE_ENDIF);
>> + m = (fs_inst *) m->prev;
>> +
>> + int else_movs = 0;
>> + while (else_movs < MAX_MOVS && m->opcode == BRW_OPCODE_MOV) {
>> + else_mov[else_movs] = m;
>> + m = (fs_inst *) m->prev;
>> + else_movs++;
>> + }
>> +
>> + if (m->opcode != BRW_OPCODE_ELSE)
>> + return 0;
>> + m = (fs_inst *) m->prev;
>> +
>> + int then_movs = 0;
>> + while (then_movs < MAX_MOVS && m->opcode == BRW_OPCODE_MOV) {
>> + then_mov[then_movs] = m;
>> + m = (fs_inst *) m->prev;
>> + then_movs++;
>> + }
>> +
>> + if (m->opcode != BRW_OPCODE_IF)
>> + return 0;
>> +
>> + *match = m;
>> + return MIN2(then_movs, else_movs);
>> +}
>> +
>> +/**
>> + * Try to replace IF/MOV+/ELSE/MOV+/ENDIF with SEL.
>> + *
>> + * Many GLSL shaders contain the following pattern:
>> + *
>> + * x = condition ? foo : bar
>> + *
>> + * or
>> + *
>> + * if (...) a.xyzw = foo.xyzw;
>> + * else a.xyzw = bar.xyzw;
>> + *
>> + * The compiler emits an ir_if tree for this, since each subexpression
>> might be
>> + * a complex tree that could have side-effects or short-circuit logic.
>> + *
>> + * However, the common case is to simply select one of two constants or
>> + * variable values---which is exactly what SEL is for. In this case, the
>> + * assembly looks like:
>> + *
>> + * (+f0) IF
>> + * ...
>> + * MOV dst src0
>> + * ELSE
>> + * ...
>> + * MOV dst src1
>> + * ENDIF
>> + *
>> + * where each pair of MOVs to a common destination and can be easily
>> translated
>> + * into
>> + *
>> + * (+f0) SEL dst src0 src1
>> + *
>> + * If src0 is an immediate value, we promote it to a temporary GRF.
>> + */
>> +bool
>> +fs_visitor::opt_peephole_sel()
>> +{
>> + bool progress = false;
>> +
>> + cfg_t cfg(this);
>> +
>> + for (int b = 0; b < cfg.num_blocks; b++) {
>> + bblock_t *block = cfg.blocks[b];
>> +
>> + int movs;
>> + fs_inst *if_inst, *endif_inst;
>> + fs_inst *start;
>> + fs_inst *else_mov[MAX_MOVS] = { NULL };
>> + fs_inst *then_mov[MAX_MOVS] = { NULL };
>> + bool bb_progress = false;
>> +
>> + /* IF and ENDIF instructions, by definition, can only be found at
>> the
>> + * ends of basic blocks.
>> + */
>> + start = (fs_inst *) block->end;
>> + if (start->opcode == BRW_OPCODE_ENDIF) {
>> + fs_inst *match = endif_inst = start;
>> +
>> + /* Find MOVs to a common destination. */
>> + movs = match_movs_from_endif(then_mov, else_mov, &match);
>> + if (movs == 0)
>> + continue;
>> +
>> + if_inst = match;
>> + } else {
>> + continue;
>> + }
>> +
>> + assert(if_inst && endif_inst);
>> +
>> + fs_inst *sel_inst[MAX_MOVS] = { NULL };
>> + fs_inst *mov_imm_inst[MAX_MOVS] = { NULL };
>> +
>> + /* Generate SEL instructions for pairs of MOVs to a common
>> destination. */
>> + for (int i = 0; i < movs; i++) {
>> + if (!then_mov[i] || !else_mov[i])
>> + break;
>> +
>> + /* Check that the MOVs are the right form. */
>> + if (!then_mov[i]->dst.equals(else_mov[i]->dst) ||
>> + then_mov[i]->is_partial_write() ||
>> + else_mov[i]->is_partial_write()) {
>> + bb_progress = false;
>> + break;
>> + }
>> +
>> + /* Only the last source register can be a constant, so if the
>> MOV in
>> + * the "then" clause uses a constant, we need to put it in a
>> + * temporary.
>> + */
>> + fs_reg src0(then_mov[i]->src[0]);
>> + if (src0.file == IMM) {
>> + src0 = fs_reg(this, glsl_type::float_type);
>> + src0.type = then_mov[i]->src[0].type;
>> + mov_imm_inst[i] = MOV(src0, then_mov[i]->src[0]);
>> + }
>
>
> Another thought: if src0 is immediate but src1 isn't, then we could avoid
> having to generate an extra instruction by reversing the arguments on the
> SEL instruction and inverting the predicate, i.e. instead of optimizing
> this:
>
> (+f0) IF
> MOV dst, 1.0
> ELSE
> MOV dst, x
> ENDIF
>
> to this:
>
> MOV tmp, 1.0
> (+f0) SEL dst, tmp, x
>
> we optimize it to this:
>
> (-f0) SEL dst, x, 1.0
I haven't written the code yet (I want to see what value numbering
does first) but I have a plan for this.
I've seen code that does
foo = bar == 0.0 ? vec4(a, b, 0.0, 1.0) : vec4(1.0, a, 1.0, 0.0)
At the end of this series, we would generate something like
(+f0) SEL dst0 a 1.0
(+f0) SEL dst1 b a
MOV tmp0 0.0
(+f0) SEL dst2 tmp0 1.0
MOV tmp1 1.0
(+f0) SEL dst3 tmp1 0.0
But instead of the last four instructions, we could just generate
MOV tmp0 0.0
(+f0) SEL dst2 tmp0 1.0
(-f0) SEL dst3 tmp0 1.0
So, basically at the time we emit the SELs and immediate MOVs, look at
what our immediate sources are and make some smarter choices.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
