Kenneth Graunke <[email protected]> writes: > On Friday, May 13, 2016 1:21:03 PM PDT Francisco Jerez wrote: >> Samuel Iglesias Gonsálvez <[email protected]> writes: >> >> > With the inclusion of the "df" field in the union, this union is going >> > to be at the offset 8 because of the alignment rules. The alignment >> > bits in the middle are uninitialized and valgrind complains with errors >> > similar to this: >> > >> > ==10298== Conditional jump or move depends on uninitialised value(s) >> > ==10298== at 0x4C31D52: __memcmp_sse4_1 (in /usr/lib/valgrind/ > vgpreload_memcheck-amd64-linux.so) >> > ==10298== by 0xAB16663: backend_reg::equals(backend_reg const&) const > (brw_shader.cpp:690) >> > ==10298== by 0xAAB629D: fs_reg::equals(fs_reg&) const (brw_fs.cpp:456) >> > ==10298== by 0xAAD4452: operands_match(fs_inst*, fs_inst*, bool*) > (brw_fs_cse.cpp:161) >> > ==10298== by 0xAAD46C3: instructions_match(fs_inst*, fs_inst*, bool*) > (brw_fs_cse.cpp:187) >> > ==10298== by 0xAAD4BAA: fs_visitor::opt_cse_local(bblock_t*) > (brw_fs_cse.cpp:251) >> > ==10298== by 0xAAD5216: fs_visitor::opt_cse() (brw_fs_cse.cpp:361) >> > ==10298== by 0xAAC8AAD: fs_visitor::optimize() (brw_fs.cpp:5401) >> > ==10298== by 0xAACB9DC: fs_visitor::run_fs(bool) (brw_fs.cpp:5803) >> > ==10298== by 0xAACC38B: brw_compile_fs (brw_fs.cpp:6029) >> > ==10298== by 0xAA39796: brw_codegen_wm_prog (brw_wm.c:137) >> > ==10298== by 0xAA3B068: brw_fs_precompile (brw_wm.c:637) >> > >> > This patch adds an explicit padding and initializes it to zero. >> > >> > Signed-off-by: Samuel Iglesias Gonsálvez <[email protected]> >> > --- >> > >> > This patch replaces the following one: >> > >> > [PATCH 2/2] i965: check each field separately in backend_end::equals() >> > >> > src/mesa/drivers/dri/i965/brw_reg.h | 5 ++++- >> > 1 file changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/ > i965/brw_reg.h >> > index 3b76d7d..ebb7f29 100644 >> > --- a/src/mesa/drivers/dri/i965/brw_reg.h >> > +++ b/src/mesa/drivers/dri/i965/brw_reg.h >> > @@ -243,6 +243,9 @@ struct brw_reg { >> > unsigned subnr:5; /* :1 in align16 */ >> > unsigned nr:16; >> > >> > + /* IMPORTANT: adjust padding bits if you add new fields */ >> > + unsigned padding:32; >> > + >> >> Ugh! It seems terribly fragile to me to make assumptions about the >> amount of (implementation-defined) padding that you're going to end up >> with. It would be awful if someone builds the driver on a different >> compiler or architecture that happens to align things differently, what >> would cause the whole compiler back-end to behave non-deterministically >> (possibly without any obvious sign of anything being wrong other than >> decreased shader performance). I think the two least insane >> possibilities we have to fix the problem are: >> >> - memset() the whole struct at the top of brw_reg() and anywhere else a >> brw_reg struct is initialized. > > This would still break in the case of: > > struct brw_reg foo = brw_imm_df(-1.0); // imm.df = 0xBFF0000000000000 > struct brw_reg bar = brw_imm_df(-2.0); // imm.df = 0xC000000000000000 > > foo.type = BRW_REGISTER_TYPE_D; > bar.type = BRW_REGISTER_TYPE_D; > foo.f = 123; > bar.f = 123; > > Here, the values are the same, but the top 32 bits are different garbage. > Initialized, but irrelevant. > Yeah, good point -- Let's go with the other approach in that case.
>> - Accept the reality that the struct contains some amount of undefined >> padding and define a helper function (e.g. brw_regs_equal() in >> brw_reg.h) to do the comparison manually, then use it everywhere we >> currently use memcmp() to compare brw_regs. > > I think this is the best approach. > >> Any suggestions Matt?
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
