On Wed, Oct 31, 2018 at 5:21 PM Timothy Arceri <tarc...@itsqueeze.com> wrote:
> On 1/11/18 1:28 am, Jason Ekstrand wrote: > > On Tue, Oct 30, 2018 at 9:17 PM Timothy Arceri <tarc...@itsqueeze.com > > <mailto:tarc...@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 > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev