Re: Need sanity check on DSE vs expander issue

[Jeff Law <law at redhat dot com>]

On Fri, 2019-12-20 at 12:08 +0100, Richard Biener wrote:
> On December 20, 2019 8:25:18 AM GMT+01:00, Jeff Law <law@redhat.com> wrote:
> > On Fri, 2019-12-20 at 08:09 +0100, Richard Biener wrote:
> > > On December 20, 2019 3:20:40 AM GMT+01:00, Jeff Law <law@redhat.com>
> > wrote:
> > > > I need a sanity check here.
> > > > 
> > > > Given this code:
> > > > 
> > > > > typedef union { long double value; unsigned int word[4]; }
> > > > memory_long_double;
> > > > > static unsigned int ored_words[4];
> > > > > static void add_to_ored_words (long double x)
> > > > > {
> > > > >   memory_long_double m;
> > > > >   size_t i;
> > > > >   memset (&m, 0, sizeof (m));
> > > > >   m.value = x;
> > > > >   for (i = 0; i < 4; i++)
> > > > >     {
> > > > >       ored_words[i] |= m.word[i];
> > > > >     }
> > > > > }
> > > > > 
> > > > 
> > > > DSE is removing the memset as it thinks the assignment to m.value
> > is
> > > > going to set the entire union.
> > > > 
> > > > But when we translate that into RTL we use XFmode:
> > > > 
> > > > > ;; m.value ={v} x_6(D);
> > > > > 
> > > > > (insn 7 6 0 (set (mem/v/j/c:XF (plus:DI (reg/f:DI 77
> > > > virtual-stack-vars)
> > > > >                 (const_int -16 [0xfffffffffffffff0])) [2
> > m.value+0
> > > > S16 A128])
> > > > >         (reg/v:XF 86 [ x ])) "j.c":13:11 -1
> > > > >      (nil))
> > > > > 
> > > > 
> > > > That (of course) only writes 80 bits of data because of XFmode,
> > leaving
> > > > 48 bits uninitialized.  We then read those bits, or-ing the
> > > > uninitialized data into ored_words and all hell breaks loose later.
> > > > 
> > > > Am I losing my mind?  ISTM that dse and the expander have to agree
> > on
> > > > how much data is written by the store to m.value.
> > > 
> > > It looks like MEM_SIZE is wrong here, so you need to figure how we
> > arrive at this (I guess TYPE_SIZE vs. MODE_SIZE mismatch is biting us
> > here?)
> > > That is, either the MEM should have BLKmode or the mode size should
> > match
> > > MEM_SIZE. Maybe DSE can avoid looking at MEM_SIZE for non-BLKmode
> > MEMs? 
> > It's gimple DSE that removes the memset, so it shouldn't be mucking
> > around with modes at all.  stmt_kills_ref_p seems to think the
> > assignment to m.value sets all of m.
> > 
> > The ao_ref for memset looks reasonable:
> > 
> > > (gdb) p *ref
> > > $14 = {ref = 0x0, base = 0x7ffff7ffbea0, offset = {<poly_int_pod<1,
> > long>> = {coeffs = {0}}, <No data fields>}, 
> > >   size = {<poly_int_pod<1, long>> = {coeffs = {128}}, <No data
> > fields>}, max_size = {<poly_int_pod<1, long>> = {
> > >       coeffs = {128}}, <No data fields>}, ref_alias_set = 0,
> > base_alias_set = 0, volatile_p = false}
> > 128 bits with a base of VAR_DECL m.
> > 
> > We looking to see if this statement will kill the ref:
> > 
> > > (gdb) p debug_gimple_stmt (stmt)
> > > # .MEM_8 = VDEF <.MEM_6>
> > > m.value ={v} x_7(D);
> > > $21 = void
> > > (gdb) p debug_tree (lhs)
> > >  <component_ref 0x7fffea97da50
> > >     type <real_type 0x7fffea988690 long double sizes-gimplified
> > volatile XF
> > >         size <integer_cst 0x7fffea7f3d20 constant 128>
> > >         unit-size <integer_cst 0x7fffea7f3d38 constant 16>
> > >         align:128 warn_if_not_align:0 symtab:0 alias-set -1
> > canonical-type 0x7fffea988690 precision:80>
> > >     side-effects volatile
> > >     arg:0 <var_decl 0x7ffff7ffbea0 m
> > >         type <union_type 0x7fffea9882a0 memory_long_double
> > sizes-gimplified volatile type_0 BLK size <integer_cst 0x7fffea7f3d20
> > 128> unit-size <integer_cst 0x7fffea7f3d38 16>
> > >             align:128 warn_if_not_align:0 symtab:0 alias-set -1
> > canonical-type 0x7fffea988348 fields <field_decl 0x7fffea9527b8 value>
> > context <translation_unit_decl 0x7fffea974168 j.i>
> > >             pointer_to_this <pointer_type 0x7fffea9883f0>>
> > >         side-effects addressable volatile used read BLK j.c:10:31
> > size <integer_cst 0x7fffea7f3d20 128> unit-size <integer_cst
> > 0x7fffea7f3d38 16>
> > >         align:128 warn_if_not_align:0 context <function_decl
> > 0x7fffea97bd00 add_to_ored_words>
> > >         chain <var_decl 0x7ffff7ffbf30 i type <integer_type
> > 0x7fffea9430a8 size_t>
> > >             used unsigned read DI j.c:11:10
> > >             size <integer_cst 0x7fffea7f3cd8 constant 64>
> > >             unit-size <integer_cst 0x7fffea7f3cf0 constant 8>
> > >             align:64 warn_if_not_align:0 context <function_decl
> > 0x7fffea97bd00 add_to_ored_words>>>
> > >     arg:1 <field_decl 0x7fffea9527b8 value
> > >         type <real_type 0x7fffea8133f0 long double sizes-gimplified
> > XF size <integer_cst 0x7fffea7f3d20 128> unit-size <integer_cst
> > 0x7fffea7f3d38 16>
> > >             align:128 warn_if_not_align:0 symtab:0 alias-set -1
> > canonical-type 0x7fffea8133f0 precision:80
> > >             pointer_to_this <pointer_type 0x7fffea813930>>
> > >         XF j.c:6:29 size <integer_cst 0x7fffea7f3d20 128> unit-size
> > <integer_cst 0x7fffea7f3d38 16>
> > >         align:128 warn_if_not_align:0 offset_align 128
> > >         offset <integer_cst 0x7fffea7f3d08 constant 0>
> > >         bit-offset <integer_cst 0x7fffea7f3d50 constant 0> context
> > <union_type 0x7fffea981e70>
> > >         chain <field_decl 0x7fffea952850 word type <array_type
> > 0x7fffea981f18>
> > >             TI j.c:6:49 size <integer_cst 0x7fffea7f3d20 128>
> > unit-size <integer_cst 0x7fffea7f3d38 16>
> > >             align:32 warn_if_not_align:0 offset_align 128 offset
> > <integer_cst 0x7fffea7f3d08 0> bit-offset <integer_cst 0x7fffea7f3d50
> > 0> context <union_type 0x7fffea981e70>>>
> > >     j.c:13:4 start: j.c:13:3 finish: j.c:13:9>
> > > $22 = void
> > > 
> > 
> > stmt_kills_ref_p calls get_ref_base_and_extent on that LHS object.  THe
> > returned base is the same as ref->base.  The returned offset is zero
> > with size/max_size of 128 bits.  So according to
> > get_ref_base_and_extent the assignment is going to write 128 bits and
> > thus kills  the memset.
> > 
> > One might argue that's where the problems start -- somewhere in
> > get_ref_base_and_extent.  
> > 
> > I'm largely offline the next couple weeks...
> > 
> > I don't have any "real" failures I'm tracking because of this, but it
> > does cause some configure generated tests to give the wrong result. 
> > Thankfully the inconsistency just doesn't matter for any of the
> > packages that are affected.
> 
> It's certainly something to look at. I'm largely offline already so please file a bug report so we don't forget. I'll have a detailed look next year. 
> 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93270

Jeff