On 10/08/2016 09:33 AM, Eduardo Lima Mitev wrote: > On 10/08/2016 02:12 AM, Ian Romanick wrote: >> From: Ian Romanick <[email protected]> >> >> This was found partially by inspection and partially by hitting a >> problem while working on nir_op_pack_int64_2x32_split. The code >> previously would 'continue' if (instr->src[i].src.is_ssa), but the code >> immediately following in the loop treats instr->src[i] as an SSA value. >> >> Signed-off-by: Ian Romanick <[email protected]> >> Cc: [email protected] >> Cc: Iago Toral Quiroga <[email protected]> >> --- >> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> index 4e68ffb..2cbcab1 100644 >> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> @@ -1208,7 +1208,7 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, >> nir_alu_instr *instr) >> * the unpack operation. >> */ >> for (int i = 0; i < 2; i++) { >> - if (instr->src[i].src.is_ssa) >> + if (!instr->src[i].src.is_ssa) >> continue; >> > > Good catch!
But maybe not. Re-running this through the CI shows about 1,000 test
regressions due to assertion failures. I looked at the rest of the
loop, and I'm really not sure how this works:
for (int i = 0; i < 2; i++) {
if (instr->src[i].src.is_ssa)
continue;
const nir_instr *parent_instr = instr->src[i].src.ssa->parent_instr;
We skip this if the source is SSA, but then we use it as SSA.
if (parent_instr->type == nir_instr_type_alu)
continue;
const nir_alu_instr *alu_parent = nir_instr_as_alu(parent_instr);
We skip this if the parent is ALU, but then we use it as ALU. The
assertion failure occurs inside nir_instr_as_alu.
if (alu_parent->op == nir_op_unpack_double_2x32_split_x ||
alu_parent->op == nir_op_unpack_double_2x32_split_y)
continue;
if (!alu_parent->src[0].src.is_ssa)
continue;
op[i] = get_nir_src(alu_parent->src[0].src);
op[i] = offset(retype(op[i], BRW_REGISTER_TYPE_DF), bld,
alu_parent->src[0].swizzle[channel]);
if (alu_parent->op == nir_op_unpack_double_2x32_split_y)
op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 1);
else
op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 0);
}
Were you guys ever able to make this optimization trigger? I suspect
that the very first continue always occurs, so none of this actually
happens. Either way, it seems like this optimization should happen in
nir_opt_algebraic instead.
This has come up because I need to do something similar for int64. All
of the lowering passes for int64 will generate a lot of
unpack(pack(...)) type sequences. I'm doing the lowering in GLSL IR,
so I've also done the algebraic optimization in GLSL IR.
> Both patches are:
>
> Reviewed-by: Eduardo Lima Mitev <[email protected]>
>
>> const nir_instr *parent_instr =
>> instr->src[i].src.ssa->parent_instr;
>>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
