On Tue, Jul 26, 2016 at 1:19 PM, Kenneth Graunke <kenn...@whitecape.org> wrote: > The pass I introduced in commit a2dc11a7818c04d8dc0324e8fcba98d60bae > was entirely broken. A missing "break" made the load_interpolated_input > case always fall through to "default" and hit a "continue", making it > not actually move any load_interpolated_input intrinsics at all. > It would only move the simple load_barycentric_* intrinsics, which > don't emit any code anyway, making it basically useless. > > The initial version I sent of the pass worked, but I apparently > failed to verify that the simplified version in v2 actually worked. > > With the obvious fix applied (so we actually tried to move LIIs),
I'm guessing that an "LII" is a "load interpolation intrinsic" or similar, but I think it'd be better to not abbreviate it since it's not a common abbreviation. > I discovered a second bug: we weren't moving the offset SSA def > to the top, breaking SSA validation. > > The new version of the pass actually moves load_interpolated_input > intrinsics and all their dependencies, as intended. > > Papers over GPU hangs on Ivybridge and Baytrail caused by the > recent NIR FS input rework by restoring the old behavior. > (I'm not honestly sure why they hang with PLN not at the top.) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97083 > Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> > --- > src/mesa/drivers/dri/i965/brw_fs.cpp | 49 > ++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 21 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp > b/src/mesa/drivers/dri/i965/brw_fs.cpp > index f9af525..bcd08ac 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > @@ -6353,38 +6353,45 @@ move_interpolation_to_top(nir_shader *nir) > continue; > > nir_block *top = nir_start_block(f->impl); > + exec_node *cursor_node = NULL; > > nir_foreach_block(block, f->impl) { > if (block == top) > continue; > > - nir_foreach_instr_reverse_safe(instr, block) { > + nir_foreach_instr_safe(instr, block) { > if (instr->type != nir_instr_type_intrinsic) > continue; > > nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr); > - switch (intrin->intrinsic) { > - case nir_intrinsic_load_barycentric_pixel: > - case nir_intrinsic_load_barycentric_centroid: > - case nir_intrinsic_load_barycentric_sample: > - break; > - case nir_intrinsic_load_interpolated_input: { > - nir_intrinsic_instr *bary_intrinsic = > - nir_instr_as_intrinsic(intrin->src[0].ssa->parent_instr); > - nir_intrinsic_op op = bary_intrinsic->intrinsic; > - > - /* Leave interpolateAtSample/Offset() where it is. */ > - if (op == nir_intrinsic_load_barycentric_at_sample || > - op == nir_intrinsic_load_barycentric_at_offset) > - continue; > - } > - default: > + if (intrin->intrinsic != nir_intrinsic_load_interpolated_input) > + continue; > + nir_intrinsic_instr *bary_intrinsic = > + nir_instr_as_intrinsic(intrin->src[0].ssa->parent_instr); > + nir_intrinsic_op op = bary_intrinsic->intrinsic; > + > + /* Leave interpolateAtSample/Offset() where they are. */ > + if (op == nir_intrinsic_load_barycentric_at_sample || > + op == nir_intrinsic_load_barycentric_at_offset) > continue; > - } > > - exec_node_remove(&instr->node); > - exec_list_push_head(&top->instr_list, &instr->node); > - instr->block = top; > + nir_instr *move[3] = { > + &bary_intrinsic->instr, > + intrin->src[1].ssa->parent_instr, > + instr > + }; > + > + for (int i = 0; i < 3; i++) { s/3/ARRAY_SIZE(move)/ ? > + if (move[i]->block != top) { > + move[i]->block = top; > + exec_node_remove(&move[i]->node); > + if (cursor_node) > + exec_node_insert_after(cursor_node, &move[i]->node); > + else > + exec_list_push_head(&top->instr_list, &move[i]->node); I'd personally use braces here. > + cursor_node = &move[i]->node; > + } > + } > } > } > nir_metadata_preserve(f->impl, (nir_metadata) > -- > 2.9.0 The logic all looks right. Reviewed-by: Matt Turner <matts...@gmail.com> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
