Need sanity check on DSE vs expander issue

[Richard Biener]

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. 

Thanks, 
Richard. 

>
>Jeff