[Bug rtl-optimization/62004] New: dead type-unsafe load replaces type-safe load

[Bug rtl-optimization/62004] New: dead type-unsafe load replaces type-safe load

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

            Bug ID: 62004
           Summary: dead type-unsafe load replaces type-safe load
           Product: gcc
           Version: 4.8.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vries at gcc dot gnu.org

Created attachment 33230
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33230&action=edit
test-case

I've tried to write a program in which there is a type-unsafe load, which is
never executed, to see if tail-merge would fail. In other words, I've tried to
come up with a 'load' variant of PR61964.

With attached test-case load-4.c and current 4.8 branch, I get the following
results:
...
$ gcc -O2 load-4.c; ./a.out ; echo $?
1
...

Adding -fno-strict-aliasing allows the test to pass:
...
$ gcc -O2 load-4.c -fno-strict-aliasing ; ./a.out ; echo $?
0
...
However AFAICT, the test-case is correct, in the sense that the only
type-unsafe code is dead, so -fno-strict-aliasing should not be necessary to
allow the test to pass.

My intention was to trigger a a problem in tail-merge. However, skipping
tail-merge still doesn't make the test pass:
...
$ gcc -O2 load-4.c -fstrict-aliasing -fno-tree-tail-merge; ./a.out ; echo $?
1
...

At rtl level, the same type of optimization as tail-merge is done. We start out
with the if-then-else-join before expand:
...
  if (_13 == h_10)
    goto <bb 3>;
  else
    goto <bb 4>;

  <bb 3>:
  p_14 = MEM[(struct head *)_13].first;
  goto <bb 5>;

  <bb 4>:
  p_15 = _13->next;

  <bb 5>:
...

And this is expanded into rtl:
...
(jump_insn 23 22 24 2 (set (pc)
        (if_then_else (ne (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (label_ref 28)
            (pc))) load-4.c:44 -1
     (expr_list:REG_BR_PROB (const_int 8986 [0x231a])
        (nil))
 -> 28)
(note 24 23 25 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(insn 25 24 26 4 (set (reg/v/f:DI 59 [ p ])
        (mem/f:DI (reg/f:DI 66 [ D.1751 ]) [4 MEM[(struct head *)_13].first+0
S8 A64])) load-4.c:46 -1
     (nil))
(jump_insn 26 25 27 4 (set (pc)
        (label_ref 31)) -1
     (nil)
 -> 31)
(barrier 27 26 28)
(code_label 28 27 29 5 2 "" [1 uses])
(note 29 28 30 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(insn 30 29 31 5 (set (reg/v/f:DI 59 [ p ])
        (mem/f:DI (reg/f:DI 66 [ D.1751 ]) [4 _13->next+0 S8 A64])) load-4.c:49
-1
     (nil))
(code_label 31 30 32 6 3 "" [1 uses])
...

Already at into_cfglayout, the jump 26 is removed, causing the 'dead' bb4 to
become alive:
...
try_optimize_cfg iteration 1

Removing jump 26.

<SNIP>
(jump_insn 23 22 24 2 (set (pc)
        (if_then_else (ne (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (label_ref 28)
            (pc))) load-4.c:44 612 {*jcc_1}
     (expr_list:REG_BR_PROB (const_int 8986 [0x231a])
        (nil))
 -> 28)
(note 24 23 25 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
(insn 25 24 28 3 (set (reg/v/f:DI 59 [ p ])
        (mem/f:DI (reg/f:DI 66 [ D.1751 ]) [4 MEM[(struct head *)_13].first+0
S8 A64])) load-4.c:46 87 {*movdi_internal_rex64}
     (nil))
(code_label 28 25 29 4 2 "" [1 uses])
(note 29 28 30 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(insn 30 29 31 4 (set (reg/v/f:DI 59 [ p ])
        (mem/f:DI (reg/f:DI 66 [ D.1751 ]) [4 _13->next+0 S8 A64])) load-4.c:49
87 {*movdi_internal_rex64}
     (nil))
...

And after ce1, we're just left with the code from bb4:
...
IF-THEN-ELSE-JOIN block found, pass 1, test 2, then 3, else 4, join 5
changing bb of uid 30
  from 4 to 2
deleting insn with uid = 29.
deleting insn with uid = 28.
deleting block 4
Removing jump 23.

<SNIP>

(insn 30 22 33 2 (set (reg/v/f:DI 59 [ p ])
        (mem/f:DI (reg/f:DI 66 [ D.1751 ]) [4 _13->next+0 S8 A64])) load-4.c:49
87 {*movdi_internal_rex64}
     (expr_list:REG_DEAD (reg/f:DI 66 [ D.1751 ])
        (nil)))
...

Using -fno-if-conversion allows the test to pass:
...
$ gcc -O2 load-4.c -fstrict-aliasing -fno-tree-tail-merge -fno-if-conversion;
./a.out ; echo $?
0
...

And indeed, the problem also triggers for tail-merge:
...
$ gcc.sh -O2 load-4.c -fstrict-aliasing -ftree-tail-merge -fno-if-conversion;
./a.out ; echo $?
1
...

[Bug rtl-optimization/62004] dead type-unsafe load replaces type-safe load

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-08-04
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Heh, interesting set of events ;)

Now it is interesting how much we desire to perform the tail-merging - we
_could_
change the alias sets of loads (and stores...) to a "common" one (either if
they
are "equal" or just zero otherwise).  Depends on how much we like this kind
of pessimization.

Same for the RTL bits of course.

Btw, I still see the conditional execution after RTL expansion, just
cfglayout mode doesn't have unconditonal gotos for the edges.

[Bug rtl-optimization/62004] dead type-unsafe load replaces type-safe load

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

--- Comment #2 from vries at gcc dot gnu.org ---
Created attachment 33242
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33242&action=edit
patch to fix if-conversion

(In reply to Richard Biener from comment #1)
> Heh, interesting set of events ;)
> 
> Now it is interesting how much we desire to perform the tail-merging - we
> _could_
> change the alias sets of loads (and stores...) to a "common" one (either if
> they
> are "equal" or just zero otherwise).  Depends on how much we like this kind
> of pessimization.
> 
> Same for the RTL bits of course.
> 
> Btw, I still see the conditional execution after RTL expansion, just
> cfglayout mode doesn't have unconditonal gotos for the edges.

Right, when doing fdump-rtl-all, it looks like fallthrough, but it isn't, I
forgot. So it's just if-conversion that does the wrong thing.

Attached patch fixes 4.8 if-conversion in a conservative way (I suppose we want
a conservative fix for 4.8 and 4.9). OK for testing?

[Bug rtl-optimization/62004] dead type-unsafe load replaces type-safe load

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #1)
> Heh, interesting set of events ;)

I have a store version that fires on mips64 with a modified testcase too, see
bug 62030.

[Bug rtl-optimization/62004] dead type-unsafe load replaces type-safe load

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

vries at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

[Bug rtl-optimization/62004] dead type-unsafe load replaces type-safe load

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

--- Comment #6 from vries at gcc dot gnu.org ---
Author: vries
Date: Fri Aug 15 21:23:21 2014
New Revision: 214044

URL: https://gcc.gnu.org/viewcvs?rev=214044&root=gcc&view=rev
Log:
Fix if-conversion pass for dead type-unsafe code

2014-08-15  Tom de Vries  <tom@codesourcery.com>

    Backport from mainline:
    2014-08-14  Tom de Vries  <tom@codesourcery.com>

    PR rtl-optimization/62004
    PR rtl-optimization/62030
    * ifcvt.c (rtx_interchangeable_p): New function.
    (noce_try_move, noce_process_if_block): Use rtx_interchangeable_p.

    * gcc.dg/pr62004.c: New test.
    * gcc.dg/pr62030.c: Same.
    * gcc.target/mips/pr62030-octeon.c: Same.

    2014-08-05  Richard Biener  <rguenther@suse.de>

    * emit-rtl.h (mem_attrs_eq_p): Declare.
    * emit-rtl.c (mem_attrs_eq_p): Export.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/gcc.dg/pr62004.c
    branches/gcc-4_9-branch/gcc/testsuite/gcc.dg/pr62030.c
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/emit-rtl.c
    branches/gcc-4_9-branch/gcc/emit-rtl.h
    branches/gcc-4_9-branch/gcc/ifcvt.c
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog

[Bug rtl-optimization/62004] dead type-unsafe load replaces type-safe load

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

--- Comment #7 from vries at gcc dot gnu.org ---
Author: vries
Date: Sat Aug 16 17:38:04 2014
New Revision: 214067

URL: https://gcc.gnu.org/viewcvs?rev=214067&root=gcc&view=rev
Log:
Fix if-conversion pass for dead type-unsafe code

2014-08-15  Tom de Vries  <tom@codesourcery.com>

    Backport from mainline:
    2014-08-14  Tom de Vries  <tom@codesourcery.com>

    PR rtl-optimization/62004
    PR rtl-optimization/62030
    * ifcvt.c (rtx_interchangeable_p): New function.
    (noce_try_move, noce_process_if_block): Use rtx_interchangeable_p.

    * gcc.dg/pr62004.c: New test.
    * gcc.dg/pr62030.c: Same.
    * gcc.target/mips/pr62030-octeon.c: Same.

    2014-08-05  Richard Biener  <rguenther@suse.de>

    * emit-rtl.h (mem_attrs_eq_p): Declare.
    * emit-rtl.c (mem_attrs_eq_p): Export.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr62004.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr62030.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/emit-rtl.c
    branches/gcc-4_8-branch/gcc/emit-rtl.h
    branches/gcc-4_8-branch/gcc/ifcvt.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

[Bug rtl-optimization/62004] [if-conversion] dead type-unsafe load replaces type-safe load

[vries at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62004

vries at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |alias
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
            Summary|dead type-unsafe load       |[if-conversion] dead
                   |replaces type-safe load     |type-unsafe load replaces
                   |                            |type-safe load

--- Comment #8 from vries at gcc dot gnu.org ---
if-conversion patch and test-case committed to trunk, 4.8 and 4.9.

tail-merge part filed as PR62167 - [tail-merge] dead type-unsafe load replaces
type-safe load

marking resolved, fixed