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

[Jason Ekstrand]

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>

> >> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150216/05b8021d/attachment.html>