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

[Jason Ekstrand]

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.

> 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
}

> --
> 2.3.0
>
> _______________________________________________
> 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/4a9baa9c/attachment.html>