Bug 62004

Summary: [if-conversion] dead type-unsafe load replaces type-safe load
Product: gcc Reporter: Tom de Vries <vries>
Component: rtl-optimizationAssignee: Tom de Vries <vries>
Status: RESOLVED FIXED    
Severity: normal CC: andersk, rguenth
Priority: P3 Keywords: alias, patch, wrong-code
Version: 4.8.4   
Target Milestone: ---   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2014-08-04 00:00:00
Attachments: test-case
patch to fix if-conversion

Description Tom de Vries 2014-08-04 00:22:04 UTC 
Created attachment 33230 [details]
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
...
Comment 1 Richard Biener 2014-08-04 10:04:55 UTC 
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.
Comment 2 Tom de Vries 2014-08-04 14:54:17 UTC 
Created attachment 33242 [details]
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?
Comment 3 Andrew Pinski 2014-08-06 07:06:11 UTC 
(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.
Comment 5 Tom de Vries 2014-08-14 16:14:31 UTC 
Author: vries
Date: Thu Aug 14 16:13:59 2014
New Revision: 213970

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

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.
	* emit-rtl.c (mem_attrs_eq_p): Remove static.
	* emit-rtl.h (mem_attrs_eq_p): Declare.

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

Added:
    trunk/gcc/testsuite/gcc.dg/pr62004.c
    trunk/gcc/testsuite/gcc.dg/pr62030.c
    trunk/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/emit-rtl.h
    trunk/gcc/ifcvt.c
    trunk/gcc/testsuite/ChangeLog
Comment 6 Tom de Vries 2014-08-15 21:23:54 UTC 
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
Comment 7 Tom de Vries 2014-08-16 17:38:35 UTC 
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
Comment 8 Tom de Vries 2014-08-18 08:51:46 UTC 
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