[Mesa-dev] [PATCH 1/2] i965/vec4: Handle nir_instr_type_deref to silence run-time output

[Jason Ekstrand]

On Tue, Jul 3, 2018 at 10:24 AM, Ian Romanick <idr at freedesktop.org> wrote:

> On 07/03/2018 07:09 AM, Jason Ekstrand wrote:
> > Do we do this in fs?  I'm very confused as to how any deref instructions
> > can survive this late.  They can for fs but only for images (not
> > textures). I would expect them to be killed off by DCE long before we
> > get here.
>
> I didn't debug this to great depth.  Once I found a couple shaders that
> hit this (apologies for sending the wrong shader to IRC), I ran with a
> breakpoint set at the fprintf.  Once that was triggered, I looked at the
> instruction type.  I saw that it was always nir_instr_type_deref, so I
> looked at fs_visitor::nir_emit_instr to see how it was handled there.  I
> then copied that code verbatim.
>
> I think texture accesses in the vertex shader was a red herring.
> Looking at the NIR coming out of one of the shaders, I see:
>
>
>         decl_var  INTERP_MODE_NONE vec4[4] t7
>         decl_reg vec4 32 r0[4]
>
>         ...
>
>         vec1 32 ssa_50 = deref_var &t7 (local vec4[4])
>         vec1 32 ssa_51 = deref_array &(*ssa_50)[0] (local vec4) /* &t7[0]
> */
>         vec2 32 ssa_52 = fmul ssa_5, ssa_49.zw
>         vec2 32 ssa_53 = ffma ssa_47, ssa_49.zw, ssa_52
>         vec4 32 ssa_54 = txl ssa_53 (coord), ssa_0 (lod),
>         r0[0] = imov ssa_54
>         vec1 32 ssa_55 = deref_array &(*ssa_50)[1] (local vec4) /* &t7[1]
> */
>         vec2 32 ssa_56 = fmul ssa_7, ssa_49.zw
>         vec2 32 ssa_57 = ffma ssa_47, ssa_49.zw, ssa_56
>         vec4 32 ssa_58 = txl ssa_57 (coord), ssa_0 (lod),
>         r0[1] = imov ssa_58
>         vec1 32 ssa_59 = deref_array &(*ssa_50)[2] (local vec4) /* &t7[2]
> */
>         vec2 32 ssa_60 = fmul ssa_8, ssa_49.zw
>         vec2 32 ssa_61 = ffma ssa_47, ssa_49.zw, ssa_60
>         vec4 32 ssa_62 = txl ssa_61 (coord), ssa_0 (lod),
>         r0[2] = imov ssa_62
>         vec1 32 ssa_63 = deref_array &(*ssa_50)[3] (local vec4) /* &t7[3]
> */
>         vec2 32 ssa_64 = fmul ssa_9, ssa_49.zw
>         vec2 32 ssa_65 = ffma ssa_47, ssa_49.zw, ssa_64
>         vec4 32 ssa_66 = txl ssa_65 (coord), ssa_0 (lod),
>         r0[3] = imov ssa_66
>
> Maybe inadequate DCE is the problem?
>

I think so.  Given that we're dealing with arrays, I supsect
lower_locals_to_regs is leaving them lying around.  We don't see a problem
in FS because lower_locals_to_regs does nothing there (all local variables
are gone long before thanks to lowering indirects away).  Also, FS has to
silently ignore derefs because we still use variables for
image_load_store.  I think we can just run DCE after all of our lowering
but before analyze_boolean_resolves.

--Jason



> > On July 3, 2018 00:26:32 "Ian Romanick" <idr at freedesktop.org> wrote:
> >
> >> From: Ian Romanick <ian.d.romanick at intel.com>
> >>
> >> Previously, shader-db runs on Gen5 through Haswell would spew tons of
> >> messages like:
> >>
> >>    VS instruction not yet implemented by NIR->vec4
> >>
> >> I checked several instance of this, and all of them were
> >> nir_instr_type_deref instructions in vertex shaders that had texture
> >> accesses (e.g., UnrealEngine4/EffectsCaveDemo/239.shader_test).
> >>
> >> Solution copied directly from fs_visitor::nir_emit_instr.
> >>
> >> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> >> Fixes: 5a02ffb733e nir: Rework lower_locals_to_regs to use deref
> >> instructions
> >> Cc: Jason Ekstrand <jason at jlekstrand.net>
> >> ---
> >> src/intel/compiler/brw_vec4_nir.cpp | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/intel/compiler/brw_vec4_nir.cpp
> >> b/src/intel/compiler/brw_vec4_nir.cpp
> >> index 7131fa06b4a..5f45d5b0c98 100644
> >> --- a/src/intel/compiler/brw_vec4_nir.cpp
> >> +++ b/src/intel/compiler/brw_vec4_nir.cpp
> >> @@ -168,6 +168,10 @@ vec4_visitor::nir_emit_instr(nir_instr *instr)
> >>       nir_emit_undef(nir_instr_as_ssa_undef(instr));
> >>       break;
> >>
> >> +   case nir_instr_type_deref:
> >> +      /* Derefs can exist for images but they do nothing */
> >> +      break;
> >> +
> >>    default:
> >>       fprintf(stderr, "VS instruction not yet implemented by
> >> NIR->vec4\n");
> >>       break;
> >> --
> >> 2.14.4
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180703/132edd62/attachment-0001.html>