[Mesa-dev] [PATCH 2/2] nir: allow propagation of if evaluation to all alu instructions

[Jason Ekstrand]

On Wed, Oct 31, 2018 at 5:21 PM Timothy Arceri <tarceri at itsqueeze.com>
wrote:

> On 1/11/18 1:28 am, Jason Ekstrand wrote:
> > On Tue, Oct 30, 2018 at 9:17 PM Timothy Arceri <tarceri at itsqueeze.com
> > <mailto:tarceri at itsqueeze.com>> wrote:
> >
> >     With the simplifications to this pass in a3b4cb34589e2f1a68 we
> >     can allow any alu instruction to be processed. For one this can
> >     potentially help with bcsels.
> >
> >
> > Do we want to?  I believe that this patch is correct and I agree that we
> > now can but then we get into some heuristics as to when it is or isn't
> > useful.  With iand, ior, and inot it's obviously useful because, for
> > boolean values, those instructions can be sort of half-folded, i.e. we
> > know that opt_algebraic will look at them and replace them with either
> > the other source or a constant.  With bcsel that's true if the condition
> > is used in src[0] but not for src[1] or src[2].  For other instructions,
> > it's even less obvious that it helps.
> >
> > I'm not saying I'm opposed.  Just thinking out loud.
>
> I'm not sure either. Ian (Ccing) was asking about support for bcsel when
> I first wrote the series, I held off sending this patch in the past (due
> to no shader-db change) but since I was looking at the code again I
> thought I might as well send it out and get comments.
>

bcsel src[0] is a no-brainer; we should definitely do that if we can.


> I'm happy to hold it back for now but I think we still have many
> opportunities for improving bcsels so maybe it will be useful in future.
>

What I'd suggest is that we go ahead with the generalization but continue
to gate it for now.  In particular, gate it on the current set of checks ||
(nir_instr_as_alu(src->parent_instr)->op == nir_op_b2i && src ==
&nir_instr_as_alu(src->parent_instr)->src[0].src)

I would give an r-b for that patch.  Maybe change can_propagate_through_alu
into a switch to make it look nicer?

--Jason


> >
> > --Jason
> >
> >     No shader-db change.
> >     ---
> >       src/compiler/nir/nir_opt_if.c | 20 ++++----------------
> >       1 file changed, 4 insertions(+), 16 deletions(-)
> >
> >     diff --git a/src/compiler/nir/nir_opt_if.c
> >     b/src/compiler/nir/nir_opt_if.c
> >     index 1fe95e53766..38489676e6b 100644
> >     --- a/src/compiler/nir/nir_opt_if.c
> >     +++ b/src/compiler/nir/nir_opt_if.c
> >     @@ -448,7 +448,7 @@ propagate_condition_eval(nir_builder *b, nir_if
> >     *nif, nir_src *use_src,
> >          if (!evaluate_if_condition(nif, b->cursor, &bool_value))
> >             return false;
> >
> >     -   nir_ssa_def *def[2] = {0};
> >     +   nir_ssa_def *def[4] = {0};
> >          for (unsigned i = 0; i < nir_op_infos[alu->op].num_inputs; i++)
> {
> >             if (alu->src[i].src.ssa == use_src->ssa) {
> >                def[i] = nir_imm_bool(b, bool_value);
> >     @@ -456,7 +456,7 @@ propagate_condition_eval(nir_builder *b, nir_if
> >     *nif, nir_src *use_src,
> >                def[i] = alu->src[i].src.ssa;
> >             }
> >          }
> >     -   nir_ssa_def *nalu = nir_build_alu(b, alu->op, def[0], def[1],
> >     NULL, NULL);
> >     +   nir_ssa_def *nalu = nir_build_alu(b, alu->op, def[0], def[1],
> >     def[2], def[3]);
> >
> >          /* Rewrite use to use new alu instruction */
> >          nir_src new_src = nir_src_for_ssa(nalu);
> >     @@ -469,19 +469,6 @@ propagate_condition_eval(nir_builder *b, nir_if
> >     *nif, nir_src *use_src,
> >          return true;
> >       }
> >
> >     -static bool
> >     -can_propagate_through_alu(nir_src *src)
> >     -{
> >     -   if (src->parent_instr->type == nir_instr_type_alu &&
> >     -       (nir_instr_as_alu(src->parent_instr)->op == nir_op_ior ||
> >     -        nir_instr_as_alu(src->parent_instr)->op == nir_op_iand ||
> >     -        nir_instr_as_alu(src->parent_instr)->op == nir_op_inot ||
> >     -        nir_instr_as_alu(src->parent_instr)->op == nir_op_b2i))
> >     -      return true;
> >     -
> >     -   return false;
> >     -}
> >     -
> >       static bool
> >       evaluate_condition_use(nir_builder *b, nir_if *nif, nir_src
> *use_src,
> >                              bool is_if_condition)
> >     @@ -502,7 +489,8 @@ evaluate_condition_use(nir_builder *b, nir_if
> >     *nif, nir_src *use_src,
> >             progress = true;
> >          }
> >
> >     -   if (!is_if_condition && can_propagate_through_alu(use_src)) {
> >     +   if (!is_if_condition &&
> >     +       use_src->parent_instr->type == nir_instr_type_alu) {
> >             nir_alu_instr *alu = nir_instr_as_alu(use_src->parent_instr);
> >
> >             nir_foreach_use_safe(alu_use, &alu->dest.dest.ssa) {
> >     --
> >     2.17.2
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181031/74be07be/attachment.html>