[Mesa-dev] [PATCH v2] i965/fs: Add remove_extra_rounding_modes optimization

[Alejandro Piñeiro]

On 08/09/17 01:58, Jason Ekstrand wrote:
> On Tue, Aug 29, 2017 at 11:54 PM, Alejandro Piñeiro
> <apinheiro at igalia.com <mailto:apinheiro at igalia.com>> wrote:
>
>     Although from SPIR-V point of view, rounding modes are attached to the
>     operation/destination, on i965 it is a status, so we don't need to
>     explicitly set the rounding mode if the one we want is already set.
>
>     v2: Use a single SHADER_OPCODE_RND_MODE opcode taking an immediate
>         with the rounding mode (Curro)
>
>     Signed-off-by: Jose Maria Casanova Crespo <jmcasanova at igalia.com
>     <mailto:jmcasanova at igalia.com>>
>     Signed-off-by: Alejandro Piñeiro <apinheiro at igalia.com
>     <mailto:apinheiro at igalia.com>
>
>     ---
>      src/intel/compiler/brw_fs.cpp | 41
>     +++++++++++++++++++++++++++++++++++++++++
>      src/intel/compiler/brw_fs.h   |  1 +
>      2 files changed, 42 insertions(+)
>
>     diff --git a/src/intel/compiler/brw_fs.cpp
>     b/src/intel/compiler/brw_fs.cpp
>     index 45236e3cab7..1b9ab0c0b88 100644
>     --- a/src/intel/compiler/brw_fs.cpp
>     +++ b/src/intel/compiler/brw_fs.cpp
>     @@ -3071,6 +3071,46 @@ fs_visitor::remove_duplicate_mrf_writes()
>         return progress;
>      }
>
>     +/**
>     + * Rounding modes for conversion instructions are included for each
>     + * conversion, but right now it is a state. So once it is set,
>     + * we don't need to call it again for subsequent calls.
>     + *
>     + * This is useful for vector/matrices conversions, as setting the
>     + * mode once is enough for the full vector/matrix
>     + */
>     +bool
>     +fs_visitor::remove_extra_rounding_modes()
>     +{
>     +   bool progress = false;
>     +   /* By default, the rounding mode is rte, so we can consider it
>     as the
>     +    * starting rounding mode
>     +    */
>     +   brw_rnd_mode prev_mode = BRW_RND_MODE_RTNE;
>
>
> This needs to be reset for every block.  Otherwise, you may end up
> invalidly making assumptions about the rounding mode at the start of a
> loop or on two sides of an if.  This probably means you should just
> use foreach_block and foreach_inst_safe separately.

True, Jose Maria pointed me the same on a internal review, and I
remember doing the change. It seems that it got lost on one of my
development branches.

Will try to find it, if not we can reimplement it.

>  
>
>     +   brw_rnd_mode current_mode;
>
>
> I think I'd prefer this be declared right there in the loop where it's
> used.  Maybe make it const and just call it "mode"?
>  
>
>     +
>     +   foreach_block_and_inst_safe (block, fs_inst, inst, cfg) {
>     +      if (inst->opcode == SHADER_OPCODE_RND_MODE) {
>     +         assert(inst->src[0].file == BRW_IMMEDIATE_VALUE);
>     +
>     +         current_mode = (brw_rnd_mode) inst->src[0].d;
>     +
>     +         if (current_mode == prev_mode) {
>     +            inst->remove(block);
>     +            progress = true;
>     +         } else {
>     +            prev_mode = current_mode;
>     +         }
>     +      }
>     +   }
>     +
>     +   if (progress)
>     +      invalidate_live_intervals();
>     +
>     +   return progress;
>     +}
>     +
>     +
>      static void
>      clear_deps_for_inst_src(fs_inst *inst, bool *deps, int first_grf,
>     int grf_len)
>      {
>     @@ -5748,6 +5788,7 @@ fs_visitor::optimize()
>         int pass_num = 0;
>
>         OPT(opt_drop_redundant_mov_to_flags);
>     +   OPT(remove_extra_rounding_modes);
>
>         do {
>            progress = false;
>     diff --git a/src/intel/compiler/brw_fs.h b/src/intel/compiler/brw_fs.h
>     index f1ba193de7e..b9476e69edb 100644
>     --- a/src/intel/compiler/brw_fs.h
>     +++ b/src/intel/compiler/brw_fs.h
>     @@ -150,6 +150,7 @@ public:
>         bool eliminate_find_live_channel();
>         bool dead_code_eliminate();
>         bool remove_duplicate_mrf_writes();
>     +   bool remove_extra_rounding_modes();
>
>         bool opt_sampler_eot();
>         bool virtual_grf_interferes(int a, int b);
>     --
>     2.11.0
>
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>