[Mesa-dev] [PATCH] i965/simd8vs: Fix SIMD8 atomics

[Ben Widawsky]

On Mon, Feb 16, 2015 at 09:24:34AM -0800, Jason Ekstrand wrote:
> On Feb 16, 2015 7:46 AM, "Francisco Jerez" <currojerez at riseup.net> wrote:
> >
> > Jason Ekstrand <jason at jlekstrand.net> writes:
> >
> > > On Feb 15, 2015 11:55 PM, "Ben Widawsky" <benjamin.widawsky at intel.com>
> > > wrote:
> > >>
> > >> The short version: we need to set bits in R0.7 which provide a mask to
> be
> > > used
> > >> for PS kill samples/pixels. Since the VS has no such concept, we just
> > > need to
> > >> set all 1.
> > >>
> > >> The longer version...
> > >> Execution for SIMD8 atomics is defined as follows:
> > >> SIMD8: The low 8 bits of the execution mask are ANDed with 8 bits of
> the
> > >> Pixel/Sample Mask from the message header. For the typed messages, the
> > > Slot
> > >> Group in the message descriptor selects either the low or high 8 bits.
> > > For the
> > >> untyped messages, the low 8 bits are always selected. The resulting
> mask
> > > is used
> > >> to determine which slots are read into the destination GRF register
> (for
> > > read),
> > >> or which slots are written to the surface (for write). If the header is
> > > not
> > >> present, only the low 8 bits of the execution mask are used.
> > >>
> > >> The message header for untyped messages is defined in R0.7 "This field
> > > contains
> > >> the 16-bit pixel/sample mask to be used for SIMD16 and SIMD8 messages.
> > > All 16
> > >> bits are used for SIMD16 messages.  For typed SIMD8 messages, Slot
> Group
> > > selects
> > >> which 8 bits of this field are used. For untyped SIMD8 messages, the
> low
> > > 8 bits
> > >> of this field are used." Furthermore, "The message header for the
> untyped
> > >> messages only needs to be delivered for pixel shader threads, where the
> > >> execution mask may indicate pixels/samples that are enabled only due to
> > >> derivative (LOD) calculations, but the corresponding slot on the
> surface
> > > must
> > >> not be accessed." We're not using a pixel shader here, but AFAICT, this
> > > mask is
> > >> used for all stages.
> > >>
> > >> This leaves two options, Remove the header, or make the VS code emit
> the
> > > correct
> > >> thing for the header. I believe one of the goals of using SIMD8 VS was
> to
> > > get as
> > >> much code reuse as possible, and so I chose the latter. Since the VS
> has
> > > no such
> > >> thing as kill instructions, the mask is derived simple as all 1's.
> > >
> > > Hm. This seems a little fishy. Don't we still have an execution mask for
> > > vertex shaders?  The second half of the if copies that into the bit
> field.
> > >
> >
> > That's fine.  The execution mask is part of the side-band information
> > sent together with the message payload to the data port. It's implicitly
> > ANDed with the sample mask sent in the header to compute the actual set
> > of channels that execute the atomic operation.  What the "second half of
> > the if" copies is not the execution mask but the sample mask used to
> > identify helper invocations of the fragment shader executed to
> > approximate derivatives which are required not to have side effects.
> 
> Good enough for me.  With the control flow change below,
> 
> Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
> 

Hi. Thanks for asking this, it reminded me that I should throw a comment in the
code as well - it took me a few hours to track down all the relevant parts of
the docs, and this commit message is a bit dense (and that was after several
hours to roughly figure out what was going on).

Thanks to the both of you. I will make the requested change.

> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87258
> > >> Cc: Kristian Høgsberg <krh at bitplanet.net>
> > >> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > >> ---
> > >>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> > > b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> > >> index 2a36d94..c20289f 100644
> > >> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> > >> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> > >> @@ -2994,6 +2994,9 @@ fs_visitor::emit_untyped_atomic(unsigned
> atomic_op,
> > > unsigned surf_index,
> > >>     if (uses_kill) {
> > >>        emit(MOV(component(sources[0], 7), brw_flag_reg(0, 1)))
> > >>           ->force_writemask_all = true;
> > >> +   } else if (stage == MESA_SHADER_VERTEX) {
> > >> +      emit(MOV(component(sources[0], 7),
> > >> +               brw_imm_ud(0xff)))->force_writemask_all = true;
> > >>     } else {
> > >>        emit(MOV(component(sources[0], 7),
> > >>                 retype(brw_vec1_grf(1, 7), BRW_REGISTER_TYPE_UD));
> > >
> > > Also would you mind rearranging the logic a bit here.  Something like
> this
> > > would make it more clear that the header stuff is only for vertex
> shaders:
> > >
> > > If (fragment) {
> > >    If (uses_kill) {
> > >    } else {
> > >    }
> > > } else {
> > >    // vertex stuff
> > > }
> > >
> >
> > Agreed, I had the same thought when I reviewed this but didn't care
> > enough to complain. :)
> >
> > >> --
> > >> 2.3.0
> > >>
> > >> _______________________________________________
> > >> mesa-dev mailing list
> > >> mesa-dev at lists.freedesktop.org
> > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev