Re: [Mesa-dev] [PATCH] i965/fs: New peephole optimization to flatten IF/BREAK/ENDIF.

2013-11-09 Thread Kenneth Graunke 
On 11/05/2013 02:58 PM, Paul Berry wrote:
[snip]
> Instead of this special-purpose peephole optimization, I'm wondering if
> it would be more useful to create a more general optimization pass that
> just converts the entire IF block to predicated instructions, assuming
> certain conditions hold.  Those conditions would be something like:

> - If any instruction but the last writes to the flag register, then
> don't do the optimization.
> - If there's nested control flow, then don't do the optimization.
> - If any instruction inside the IF block is incompatible with
> predication (are there any such instructions?  I don't know) then don't
> do the optimization.
> - If the block is larger than a certain heuristically-determined size,
> then don't do the optimization.
> 
> Such an optimization could in principle even handle IF/ELSE blocks, by
> inverting the predicate in the ELSE section.

Eric and I actually wrote such a pass...it's sitting on a branch
somewhere.  At the time, it helped some things massively, but caused a
few shaders to explode into spilling.  Maybe with the newer scheduling
changes, that problem's been solved...

--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/fs: New peephole optimization to flatten IF/BREAK/ENDIF.

2013-11-05 Thread Paul Berry 
On 5 November 2013 14:58, Paul Berry  wrote:

> On 1 November 2013 17:31, Matt Turner  wrote:
>
>> total instructions in shared programs: 1649485 -> 1649157 (-0.02%)
>> instructions in affected programs: 7823 -> 7495 (-4.19%)
>> ---
>>  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_peephole_predicated_break.cpp  | 96
>> ++
>>  4 files changed, 99 insertions(+)
>>  create mode 100644
>> src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
>> b/src/mesa/drivers/dri/i965/Makefile.sources
>> index 14b2f61..fcc6fdb 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_peephole_predicated_break.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 7064910..5c8443f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -3149,6 +3149,7 @@ fs_visitor::run()
>>  progress = opt_algebraic() || progress;
>>  progress = opt_cse() || progress;
>>  progress = opt_copy_propagate() || progress;
>> + progress = opt_peephole_predicated_break() || 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 43e4761..09e5174 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -362,6 +362,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_predicated_break();
>> 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_peephole_predicated_break.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
>> new file mode 100644
>> index 000..7845113
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
>> @@ -0,0 +1,96 @@
>> +/*
>> + * 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_peephole_predicated_break.cpp
>> + *
>> + * Loops are often structured as
>> + *
>> + * loop:
>> + *CMP.f0
>> + *(+f0) IF
>> + *BREAK
>> + *ENDIF
>> + *...
>> + *WHILE loop
>> + *
>> + * This peephole pass removes the IF and ENDIF instructions and
>> predicates the
>> + * BREAK, dropping two instructions from the loop body.
>>
>
> Instead of this special-purpose peephole optimization, I'm wondering if it
> would be more useful to create a more general optimization pass that just
> converts the entire IF block to predicated instructions, assuming certain
> conditions hold.  Those conditions would be something like:
>
> - If any instruction but the last writes to the flag register, then don't
> do the optimization.
> - If there's nested control flow, then don't do the optimization.
> - If any instruction inside the IF block is incompatible with predic

Re: [Mesa-dev] [PATCH] i965/fs: New peephole optimization to flatten IF/BREAK/ENDIF.

2013-11-05 Thread Paul Berry 
On 1 November 2013 17:31, Matt Turner  wrote:

> total instructions in shared programs: 1649485 -> 1649157 (-0.02%)
> instructions in affected programs: 7823 -> 7495 (-4.19%)
> ---
>  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_peephole_predicated_break.cpp  | 96
> ++
>  4 files changed, 99 insertions(+)
>  create mode 100644
> src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
>
> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
> b/src/mesa/drivers/dri/i965/Makefile.sources
> index 14b2f61..fcc6fdb 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_peephole_predicated_break.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 7064910..5c8443f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -3149,6 +3149,7 @@ fs_visitor::run()
>  progress = opt_algebraic() || progress;
>  progress = opt_cse() || progress;
>  progress = opt_copy_propagate() || progress;
> + progress = opt_peephole_predicated_break() || 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 43e4761..09e5174 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -362,6 +362,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_predicated_break();
> 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_peephole_predicated_break.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
> new file mode 100644
> index 000..7845113
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
> @@ -0,0 +1,96 @@
> +/*
> + * 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_peephole_predicated_break.cpp
> + *
> + * Loops are often structured as
> + *
> + * loop:
> + *CMP.f0
> + *(+f0) IF
> + *BREAK
> + *ENDIF
> + *...
> + *WHILE loop
> + *
> + * This peephole pass removes the IF and ENDIF instructions and
> predicates the
> + * BREAK, dropping two instructions from the loop body.
>

Instead of this special-purpose peephole optimization, I'm wondering if it
would be more useful to create a more general optimization pass that just
converts the entire IF block to predicated instructions, assuming certain
conditions hold.  Those conditions would be something like:

- If any instruction but the last writes to the flag register, then don't
do the optimization.
- If there's nested control flow, then don't do the optimization.
- If any instruction inside the IF block is incompatible with predication
(are there any such instructions?  I don't know) then don't do the
optimization.
- If the block is larger than a certain heuristically-determined size, then
don't do the optimiza

[Mesa-dev] [PATCH] i965/fs: New peephole optimization to flatten IF/BREAK/ENDIF.

2013-11-01 Thread Matt Turner 
total instructions in shared programs: 1649485 -> 1649157 (-0.02%)
instructions in affected programs: 7823 -> 7495 (-4.19%)
---
 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_peephole_predicated_break.cpp  | 96 ++
 4 files changed, 99 insertions(+)
 create mode 100644 
src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp

diff --git a/src/mesa/drivers/dri/i965/Makefile.sources 
b/src/mesa/drivers/dri/i965/Makefile.sources
index 14b2f61..fcc6fdb 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_peephole_predicated_break.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 7064910..5c8443f 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -3149,6 +3149,7 @@ fs_visitor::run()
 progress = opt_algebraic() || progress;
 progress = opt_cse() || progress;
 progress = opt_copy_propagate() || progress;
+ progress = opt_peephole_predicated_break() || 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 43e4761..09e5174 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.h
+++ b/src/mesa/drivers/dri/i965/brw_fs.h
@@ -362,6 +362,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_predicated_break();
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_peephole_predicated_break.cpp 
b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
new file mode 100644
index 000..7845113
--- /dev/null
+++ b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
@@ -0,0 +1,96 @@
+/*
+ * 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_peephole_predicated_break.cpp
+ *
+ * Loops are often structured as
+ *
+ * loop:
+ *CMP.f0
+ *(+f0) IF
+ *BREAK
+ *ENDIF
+ *...
+ *WHILE loop
+ *
+ * This peephole pass removes the IF and ENDIF instructions and predicates the
+ * BREAK, dropping two instructions from the loop body.
+ */
+
+bool
+fs_visitor::opt_peephole_predicated_break()
+{
+   bool progress = false;
+
+   cfg_t cfg(this);
+
+   for (int b = 0; b < cfg.num_blocks; b++) {
+  bblock_t *block = cfg.blocks[b];
+
+  /* IF instructions, by definition, can only be found at the ends of
+   * basic blocks.
+   */
+  fs_inst *if_inst = (fs_inst *) block->end;
+  if (if_inst->opcode != BRW_OPCODE_IF)
+ continue;
+
+  fs_inst *inst = (fs_inst *) if_inst->next;
+  if (inst->opcode != BRW_OPCODE_BREAK && inst->opcode != 
BRW_OPCODE_CONTINUE)
+ continue;
+
+  fs_inst *endif_inst = (fs_inst *) inst->next;
+  if (endif_inst->opcode != BRW_OPCODE_ENDIF)
+ continue;
+
+  /* For Sandybridge with IF with embedded comparison we need to emit an
+   * instruction to set the flag register.
+   */
+  if (brw->gen == 6 && if_inst->conditional_mod) {
+ fs_inst *cmp_inst = CMP(reg_null_d, if_inst->s