<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Oct 31, 2018 at 5:21 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 1/11/18 1:28 am, Jason Ekstrand wrote:<br>
> On Tue, Oct 30, 2018 at 9:17 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a> <br>
> <mailto:<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>>> wrote:<br>
> <br>
> With the simplifications to this pass in a3b4cb34589e2f1a68 we<br>
> can allow any alu instruction to be processed. For one this can<br>
> potentially help with bcsels.<br>
> <br>
> <br>
> Do we want to? I believe that this patch is correct and I agree that we <br>
> now can but then we get into some heuristics as to when it is or isn't <br>
> useful. With iand, ior, and inot it's obviously useful because, for <br>
> boolean values, those instructions can be sort of half-folded, i.e. we <br>
> know that opt_algebraic will look at them and replace them with either <br>
> the other source or a constant. With bcsel that's true if the condition <br>
> is used in src[0] but not for src[1] or src[2]. For other instructions, <br>
> it's even less obvious that it helps.<br>
> <br>
> I'm not saying I'm opposed. Just thinking out loud.<br>
<br>
I'm not sure either. Ian (Ccing) was asking about support for bcsel when <br>
I first wrote the series, I held off sending this patch in the past (due <br>
to no shader-db change) but since I was looking at the code again I <br>
thought I might as well send it out and get comments.<br></blockquote><div><br></div><div>bcsel src[0] is a no-brainer; we should definitely do that if we can.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'm happy to hold it back for now but I think we still have many <br>
opportunities for improving bcsels so maybe it will be useful in future.<br></blockquote><div><br></div><div>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)</div><div><br></div><div>I would give an r-b for that patch. Maybe change can_propagate_through_alu into a switch to make it look nicer?</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> <br>
> --Jason<br>
> <br>
> No shader-db change.<br>
> ---<br>
> src/compiler/nir/nir_opt_if.c | 20 ++++----------------<br>
> 1 file changed, 4 insertions(+), 16 deletions(-)<br>
> <br>
> diff --git a/src/compiler/nir/nir_opt_if.c<br>
> b/src/compiler/nir/nir_opt_if.c<br>
> index 1fe95e53766..38489676e6b 100644<br>
> --- a/src/compiler/nir/nir_opt_if.c<br>
> +++ b/src/compiler/nir/nir_opt_if.c<br>
> @@ -448,7 +448,7 @@ propagate_condition_eval(nir_builder *b, nir_if<br>
> *nif, nir_src *use_src,<br>
> if (!evaluate_if_condition(nif, b->cursor, &bool_value))<br>
> return false;<br>
> <br>
> - nir_ssa_def *def[2] = {0};<br>
> + nir_ssa_def *def[4] = {0};<br>
> for (unsigned i = 0; i < nir_op_infos[alu->op].num_inputs; i++) {<br>
> if (alu->src[i].src.ssa == use_src->ssa) {<br>
> def[i] = nir_imm_bool(b, bool_value);<br>
> @@ -456,7 +456,7 @@ propagate_condition_eval(nir_builder *b, nir_if<br>
> *nif, nir_src *use_src,<br>
> def[i] = alu->src[i].src.ssa;<br>
> }<br>
> }<br>
> - nir_ssa_def *nalu = nir_build_alu(b, alu->op, def[0], def[1],<br>
> NULL, NULL);<br>
> + nir_ssa_def *nalu = nir_build_alu(b, alu->op, def[0], def[1],<br>
> def[2], def[3]);<br>
> <br>
> /* Rewrite use to use new alu instruction */<br>
> nir_src new_src = nir_src_for_ssa(nalu);<br>
> @@ -469,19 +469,6 @@ propagate_condition_eval(nir_builder *b, nir_if<br>
> *nif, nir_src *use_src,<br>
> return true;<br>
> }<br>
> <br>
> -static bool<br>
> -can_propagate_through_alu(nir_src *src)<br>
> -{<br>
> - if (src->parent_instr->type == nir_instr_type_alu &&<br>
> - (nir_instr_as_alu(src->parent_instr)->op == nir_op_ior ||<br>
> - nir_instr_as_alu(src->parent_instr)->op == nir_op_iand ||<br>
> - nir_instr_as_alu(src->parent_instr)->op == nir_op_inot ||<br>
> - nir_instr_as_alu(src->parent_instr)->op == nir_op_b2i))<br>
> - return true;<br>
> -<br>
> - return false;<br>
> -}<br>
> -<br>
> static bool<br>
> evaluate_condition_use(nir_builder *b, nir_if *nif, nir_src *use_src,<br>
> bool is_if_condition)<br>
> @@ -502,7 +489,8 @@ evaluate_condition_use(nir_builder *b, nir_if<br>
> *nif, nir_src *use_src,<br>
> progress = true;<br>
> }<br>
> <br>
> - if (!is_if_condition && can_propagate_through_alu(use_src)) {<br>
> + if (!is_if_condition &&<br>
> + use_src->parent_instr->type == nir_instr_type_alu) {<br>
> nir_alu_instr *alu = nir_instr_as_alu(use_src->parent_instr);<br>
> <br>
> nir_foreach_use_safe(alu_use, &alu->dest.dest.ssa) {<br>
> -- <br>
> 2.17.2<br>
> <br>
</blockquote></div></div>