[Mesa-dev] [PATCH 2/2] i965: check each field separately in backend_end::equals()

[Francisco Jerez]

Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

> Extra bits required to make room for the df field of the union don't get
> initialized in all codepaths, so backend_reg comparisons done using
> memcmp() can basically return random results. Check field by field to
> avoid this.
>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> Reported-by: Francisco Jerez <currojerez at riseup.net>

This seemed to avoid the problem on at least the few tests I ran
manually under valgrind, but I have doubts that it is a complete fix,
grepping for 'memcmp' in the back-end gives the following uses with a
brw_reg as argument that will likely still give non-deterministic
results even with this patch applied:

brw_fs_generator.cpp:1009:      if (memcmp(&surface_reg, &sampler_reg, sizeof(surface_reg)) == 0) {
brw_vec4_generator.cpp:298:      if (memcmp(&surface_reg, &sampler_reg, sizeof(surface_reg)) == 0) {

Might be a good idea to factor out the manual comparison into a static
function in brw_reg.h you could use in the FS and VEC4 generators to
avoid duplicating the complex expression?  Or maybe find out why PATCH 1
is not sufficient to fix the issue?  Most likely something else other
than brw_reg() is attempting to initialize brw_reg structs manually?

One more comment below.

> ---
>  src/mesa/drivers/dri/i965/brw_reg.h      |  1 +
>  src/mesa/drivers/dri/i965/brw_shader.cpp | 21 +++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
> index 3b76d7d..68128bd 100644
> --- a/src/mesa/drivers/dri/i965/brw_reg.h
> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
> @@ -233,6 +233,7 @@ uint32_t brw_swizzle_immediate(enum brw_reg_type type, uint32_t x, unsigned swz)
>   * WM programs to implement shaders decomposed into "channel serial"
>   * or "structure of array" form:
>   */
> +/* IMPORTANT: update backend_reg::equals() if you add a new field here. */
>  struct brw_reg {
>     enum brw_reg_type type:4;
>     enum brw_reg_file file:3;      /* :2 hardware format */
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> index a23f14e..7871f51 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> @@ -687,8 +687,25 @@ backend_shader::backend_shader(const struct brw_compiler *compiler,
>  bool
>  backend_reg::equals(const backend_reg &r) const
>  {
> -   return memcmp((brw_reg *)this, (brw_reg *)&r, sizeof(brw_reg)) == 0 &&
> -          reg_offset == r.reg_offset;
> +   bool equal = true;

No need for this variable, you can just return false early.

> +
> +   if (this->reg_offset != r.reg_offset ||
> +       this->type != r.type ||
> +       this->file != r.file ||
> +       this->negate != r.negate ||
> +       this->abs != r.abs ||
> +       this->address_mode != r.address_mode ||
> +       this->pad0 != r.pad0 ||
> +       this->subnr != r.subnr ||
> +       this->nr != r.nr ||
> +       this->ud != r.ud)
> +      equal = false;
> +
> +   /* Check higher 32 bits of the brw_reg's union if they are filled */
> +   if (equal && this->type == BRW_REGISTER_TYPE_DF && this->df != r.df)
> +      equal = false;
> +
> +   return equal;
>  }
>  
>  bool
> -- 
> 2.5.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160512/457390fb/attachment.sig>