<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 14, 2016 at 10:23 AM, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 10/08/2016 09:33 AM, Eduardo Lima Mitev wrote:<br>
> On 10/08/2016 02:12 AM, Ian Romanick wrote:<br>
>> From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>><br>
>><br>
>> This was found partially by inspection and partially by hitting a<br>
>> problem while working on nir_op_pack_int64_2x32_split.  The code<br>
>> previously would 'continue' if (instr->src[i].src.is_ssa), but the code<br>
>> immediately following in the loop treats instr->src[i] as an SSA value.<br>
>><br>
>> Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>><br>
>> Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
>> Cc: Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>><br>
>> ---<br>
>>  src/mesa/drivers/dri/i965/brw_<wbr>fs_nir.cpp | 2 +-<br>
>>  1 file changed, 1 insertion(+), 1 deletion(-)<br>
>><br>
>> diff --git a/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp<br>
>> index 4e68ffb..2cbcab1 100644<br>
>> --- a/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp<br>
>> +++ b/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp<br>
>> @@ -1208,7 +1208,7 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)<br>
>>         * the unpack operation.<br>
>>         */<br>
>>        for (int i = 0; i < 2; i++) {<br>
>> -         if (instr->src[i].src.is_ssa)<br>
>> +         if (!instr->src[i].src.is_ssa)<br>
>>              continue;<br>
>><br>
><br>
> Good catch!<br>
<br>
</div></div>But maybe not.  Re-running this through the CI shows about 1,000 test<br>
regressions due to assertion failures.  I looked at the rest of the<br>
loop, and I'm really not sure how this works:<br>
<span class=""><br>
      for (int i = 0; i < 2; i++) {<br>
</span><span class="">         if (instr->src[i].src.is_ssa)<br>
</span>            continue;<br></blockquote><div><br></div><div>Yeah, this should be !<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
         const nir_instr *parent_instr = instr->src[i].src.ssa->parent_<wbr>instr;<br>
<br>
</span>We skip this if the source is SSA, but then we use it as SSA.<br>
<br>
         if (parent_instr->type == nir_instr_type_alu)<br></blockquote><div><br></div><div>and this should be !=<br><br></div><div>if it works at all...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            continue;<br>
<br>
         const nir_alu_instr *alu_parent = nir_instr_as_alu(parent_instr)<wbr>;<br>
<br>
We skip this if the parent is ALU, but then we use it as ALU.  The<br>
assertion failure occurs inside nir_instr_as_alu.<br>
<br>
         if (alu_parent->op == nir_op_unpack_double_2x32_<wbr>split_x ||<br>
             alu_parent->op == nir_op_unpack_double_2x32_<wbr>split_y)<br>
            continue;<br>
<br>
         if (!alu_parent->src[0].src.is_<wbr>ssa)<br>
            continue;<br>
<br>
         op[i] = get_nir_src(alu_parent->src[0]<wbr>.src);<br>
         op[i] = offset(retype(op[i], BRW_REGISTER_TYPE_DF), bld,<br>
                        alu_parent->src[0].swizzle[<wbr>channel]);<br>
         if (alu_parent->op == nir_op_unpack_double_2x32_<wbr>split_y)<br>
            op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 1);<br>
         else<br>
            op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 0);<br>
      }<br>
<br>
Were you guys ever able to make this optimization trigger?  I suspect<br>
that the very first continue always occurs, so none of this actually<br>
happens.  Either way, it seems like this optimization should happen in<br>
nir_opt_algebraic instead.<br></blockquote><div><br></div><div>I've never been a huge fan of this optimization.  My brain's too dead on a Friday to think about all of the implications, but it's obviously not working as-is.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This has come up because I need to do something similar for int64.  All<br>
of the lowering passes for int64 will generate a lot of<br>
unpack(pack(...)) type sequences.  I'm doing the lowering in GLSL IR,<br>
so I've also done the algebraic optimization in GLSL IR.<br>
<div class="HOEnZb"><div class="h5"><br>
> Both patches are:<br>
><br>
> Reviewed-by: Eduardo Lima Mitev <<a href="mailto:elima@igalia.com">elima@igalia.com</a>><br>
><br>
>>           const nir_instr *parent_instr = instr->src[i].src.ssa->parent_<wbr>instr;<br>
>><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>