Samuel Iglesias Gonsálvez <[email protected]> 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 <[email protected]> > Reported-by: Francisco Jerez <[email protected]>
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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
