[Bug tree-optimization/18628] New: miscompilation of complex switch statement

[Bug tree-optimization/18628] New: miscompilation of complex switch statement

[micis at gmx dot de]

When I compile qt-x11-free-3.3.3 with the actual snapshot using the options:
  -O3 --param max-gcse-passes=4 -floop-optimize2 -fmove-loop-invariants
I get get a miscompilation in the file sql_lex.cc in a very complex switch
statement which results in an error during linking:
  "undefined reference to .L968"

This error goes away when I ommit -floop-optimize2 -fmove-loop-invariants or
use -O2 or max-gcse-passes=3

Since the source to produce this error is too large (~ 100 .o files and 38
libraries are linking together) I did not file a bug report.

Maybe the combination of parameters doesn't make sense or is invalid, but my
goal was to stress the compiler by using parameters which might produce
better code.
(We have c++ programs where max-gcse-passes=5 is necessary to get fast code)

My question are:
Is this a valid combination of parameters and should that combination work?
Should I submit this as a bug report?


Michael Cieslinski


output gcc -v:
Reading specs from
/usr/local/gcc40a/lib/gcc/powerpc-unknown-linux-gnu/4.0.0/specs
Configured with: ../gcc40/configure --prefix=/usr/local/gcc40a
--program-suffix=40a --with-cpu=G5 --enable-altivec --enable-languages=c,c++
--enable-checking
Thread model: posix
gcc version 4.0.0 20041121 (experimental)

The source can be downloaded at:
http://www.trolltech.com/download/qt/x11.html

to compile I use CFLAGS and CXXFLAGS:
-pipe -g -O3 -mcpu=G5 -maltivec -mabi=altivec -fmessage-length=0
-falign-loops=16 -falign-jumps=16 -falign-functions=32 -fstrict-aliasing
-fsched-interblock --param max-gcse-passes=5 -ffast-math -funroll-all-loops
-funit-at-a-time -fpeel-loops -ftracer -funswitch-loops -floop-optimize2
-fmove-loop-invariants -fgcse-after-reload -frename-registers

QT was configured with:
./configure --prefix=/home/cie019/mysql/mysql --enable-thread-safe-client

-- 
           Summary: miscompilation of complex switch statement
           Product: gcc
           Version: 4.0.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: tree-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: micis at gmx dot de
                CC: gcc-bugs at gcc dot gnu dot org
 GCC build triplet: powerpc-unknown-linux-gnu
  GCC host triplet: powerpc-unknown-linux-gnu
GCC target triplet: powerpc-unknown-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug tree-optimization/18628] miscompilation of complex switch statement

[micis at gmx dot de]

------- Additional Comments From micis at gmx dot de  2004-11-23 15:14 -------
Created an attachment (id=7587)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=7587&action=view)
preprocessed source


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] miscompilation of complex switch statement

[pinskia at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pinskia at gcc dot gnu dot
                   |                            |org
          Component|tree-optimization           |middle-end
           Keywords|                            |wrong-code


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] miscompilation of complex switch statement

[reichelt at gcc dot gnu dot org]

------- Additional Comments From reichelt at gcc dot gnu dot org  2004-11-23 15:39 -------
I can confirm this on i686-pc-linux-gnu just using "-O2".


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |reichelt at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of complex switch statement

[reichelt at gcc dot gnu dot org]

------- Additional Comments From reichelt at gcc dot gnu dot org  2004-11-23 16:10 -------
Here's a reduced testcase for i686-pc-liux-gnu. Just compile with
gcc -O2 to see a linker failure:

======================================
enum { e0, e1, e2, e3, e4, e5 } e;

int main()
{
  for (;;)
  {
    switch (e)
    {
      case e0:
      case e1:
        return 1;

      case e2:
      case e3:
        return 0;

      case e5:
        e = e4;
    }
  }
}
======================================

/tmp/ccYklAp3.o(.text+0x46): In function `main':
: undefined reference to `.L10'
collect2: ld returned 1 exit status


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |critical
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|                            |1
  GCC build triplet|powerpc-unknown-linux-gnu   |
   GCC host triplet|powerpc-unknown-linux-gnu   |
 GCC target triplet|powerpc-unknown-linux-gnu   |
           Keywords|                            |monitored
      Known to fail|                            |4.0.0
      Known to work|                            |3.4.3
   Last reconfirmed|0000-00-00 00:00:00         |2004-11-23 16:10:55
               date|                            |
            Summary|miscompilation of complex   |[4.0 regression]
                   |switch statement            |miscompilation of complex
                   |                            |switch statement
   Target Milestone|---                         |4.0.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of complex switch statement

[reichelt at gcc dot gnu dot org]

------- Additional Comments From reichelt at gcc dot gnu dot org  2004-11-23 16:16 -------
Works also with int:

========================
int i;

int main()
{
  for (;;)
  {
    switch (i)
    {
      case 0:
      case 1:
        return 1;

      case 2:
      case 3:
        return 0;

      case 5:
        --i;
    }
  }
}
========================


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of complex switch statement

[reichelt at gcc dot gnu dot org]

------- Additional Comments From reichelt at gcc dot gnu dot org  2004-11-23 16:34 -------
The regression appears in gcc 3.5.0 20040712.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[reichelt at gcc dot gnu dot org]

------- Additional Comments From reichelt at gcc dot gnu dot org  2004-11-23 16:53 -------
Zdenek, the regression was introduced by your patch
http://gcc.gnu.org/ml/gcc-cvs/2004-07/msg00536.html

Could you please have a look?


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rakdver at gcc dot gnu dot
                   |                            |org
            Summary|[4.0 regression]            |[4.0 regression]
                   |miscompilation of complex   |miscompilation of switch
                   |switch statement            |statement in loop


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-11-23 17:09 -------
I really doubt that his change did anything except maybe change the tree level.
Here is one which can most likely reproduced before his change:
int main (int i)
{
  int lsm_tmp1;
  int D1127;
  lsm_tmp1 = i;
L9:;
  switch (lsm_tmp1)
    { 
      case 0 ... 1: goto L10;
      case 2 ... 3: goto L3;
      case 5: goto L5;
      default : goto L9;
    }
L3:;
  i = lsm_tmp1;
  D1127 = 0;
  goto L7;
L5:;
  lsm_tmp1 = 4;
  goto L9;
L10:;
  i = lsm_tmp1;
  D1127 = 1;
L7:;
  return D1127;
}

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-11-23 17:13 -------
In fact we just to ICE on my example back before 2004-06-18 and not produce the label after that.

It worked before 20040511 but in 2004-05-14 we produced the segfault.  (this is the time period 
which the tree-ssa was merged in).

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-11-23 17:22 -------
It started to ICE on the tree-ssa:
: Search converges between 2003-08-31-ssa (#74) and 2003-09-01-ssa (#75).
But it stopped:
: Search converges between 2003-09-01-ssa (#75) and 2003-09-03-ssa (#76).
At which point we started to produce wrong code.

This looks like a laten bug in the rtl optimizaters.

Note with -O2 -fno-gcse I cannot reproduce this.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-11-23 17:38 -------
The bug looks to be in *.ce1.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-11-23 17:48 -------
We are removing a bb which we should not be.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-11-23 18:15 -------
Well before we would move the label but now we don't.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2004-11-28 18:01 -------
Hmm... This is a difficult one.  We manage to split a table jump sequence, and
introduce a label between the indexed load of a jump table and the indirect
jump.  We then hoist the indexed load, and later simplify/GCSE/CSE it to a load
of one of its symbol_ref from the tablejump vector.  Hence we have a load of a
symbol, in a block that then jumps to the indirect jump.  Everything is
consistent at this point, even though the load of symbol_ref doesn't appear in
the CFG until we later decide to forward one of the edges from the tablejump
(but without realizing that a REG_LABEL copy has been made of it, and eventually
the incoming edge count for that block reaches zero, so we delete the block,
leaving a dangling reference.

There are several ways of addressing this issue:
[1] If we ever split a tablejump by inserting a label between its load and the
indirect jump, we need to treat it as a generic indirect jump, potentially
jumping to any (case) label in the function/program.
[2] We could prohibit the splitting of tablejumps as above.  Either by keeping
things together in the middle-end or by changing backends by only splitting the
tablejump after these optimization passes (reload?).
[3] We could prohibit the simplification that extracts a symbol ref out of a
jump table, so that an indexed load is always performed a run-time, and so
changing the jump table is always safe.
[4] Perhaps we could tag the jump table to indicate that once we've extracted a
value out of it, it's effectively constant and we can no longer redirect edges.
[5] At the point that we extract the symbol ref from the jump table, we could
mark the destination symbol as LABEL_PRESERVE_P to indicate that it shouldn't
be deleted.  Unfortunately, it looks like LABEL_PRESERVE_P isn't honored by
the try_cfg_optimize routine that deletes basic blocks, but perhaps we could
refuse to thread jumps over a label marked LABEL_PRESERVE.
[6] It might be sufficent to avoid threading jumps over basic blocks whose
LABEL_NUSES doesn't match their EDGE_CODE(preds).
[7] Perhaps tree-ssa could recognize the potential infinite loop and prevent
the RTL optimizers by getting confused by it (__builtin_infinite_loop()?).

[8] As an enhancement, if we have two consecutive labels the decision of which
to replace could be tweaked to check whether one of the labels is marked
PRESERVED or has excess NUSES.  There's also a performance issue given two
labels at the same location, we should retain the label with the most incoming
edges to minimize the number of jumps/edges that need to be redirected.


Basically, its not if-conversion that's at fault, but a bad interation between
the cfg_optimize function it calls, and the cumulative effects of the earlier
optimizations (gcse for hoisting the load, loop for introducing the empty
loop header, etc...)  All of these transformations legitimately improve
performance on real world code like bytecode interpreters, hence choosing
which one (or combination) to disable looks to be a compromise.

Anyway how best to fix this really requires the attention of a CFG guru,
perhaps either Jan or RTH.  Certainly Zdenek's patch is innocent, this is an
underlying design issue.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[steven at gcc dot gnu dot org]

------- Additional Comments From steven at gcc dot gnu dot org  2004-12-15 12:30 -------
Given Roger's analysis, this bug is probably present in earlier GCCs as 
well, so this is not really a regression. 
 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[steven at gcc dot gnu dot org]

------- Additional Comments From steven at gcc dot gnu dot org  2004-12-15 13:54 -------
Just so everyone knows, the label disappears in the call to cleanup_cfg 
from rest_of_handle_if_conversion.  The CFG just before looks like this: 
 
Basic block 0 (dirty) 
Predecessors:  ENTRY [100.0%]  (fallthru) 
Successors:  7 [100.0%] 
 
Basic block 1 
Predecessors:  2 [25.0%]  (loop_exit) 7 [50.0%] 
Successors:  7 [100.0%]  (fallthru,dfs_back) 
 
Basic block 7 (new) 
Predecessors:  1 [100.0%]  (fallthru,dfs_back) 0 [100.0%] 
Successors:  1 [50.0%]  2 [50.0%]  (fallthru) 
 
Basic block 2 
Predecessors:  7 [50.0%]  (fallthru) 4 [100.0%]  (dfs_back) 
Successors:  4 [25.0%]  1 [25.0%]  (loop_exit) 5 [25.0%]  (loop_exit) 3 
[25.0%]  (loop_exit) 
 
Basic block 3 
Predecessors:  2 [25.0%]  (loop_exit) 
Successors:  6 [100.0%] 
 
Basic block 4 
Predecessors:  2 [25.0%] 
Successors:  2 [100.0%]  (dfs_back) 
 
Basic block 5 
Predecessors:  2 [25.0%]  (loop_exit) 
Successors:  6 [100.0%]  (fallthru) 
 
Basic block 6 
Predecessors:  3 [100.0%]  5 [100.0%]  (fallthru) 
Successors:  EXIT [100.0%]  (fallthru) 
 
 
and the RTL looks like this: 
(gdb) p debug_rtx_list (get_insns (), max_uid) 
(note 2 0 20 NOTE_INSN_DELETED) 
 
(note 20 2 6 0 [bb 0] NOTE_INSN_BASIC_BLOCK) 
 
(insn 6 20 9 0 (set (reg/v:SI 61 [ i ]) 
        (mem/i:SI (reg/f:SI 16 argp) [3 i+0 S4 A32])) 42 
{*movsi_1_nointernunit} (nil) 
    (expr_list:REG_EQUIV (mem/i:SI (reg/f:SI 16 argp) [3 i+0 S4 A32]) 
        (nil))) 
 
(insn 9 6 15 0 (parallel [ 
            (set (reg/f:SI 7 sp) 
                (and:SI (reg/f:SI 7 sp) 
                    (const_int -16 [0xfffffffffffffff0]))) 
            (clobber (reg:CC 17 flags)) 
        ]) 289 {*andsi_1} (nil) 
    (nil)) 
 
(insn 15 9 7 0 (parallel [ 
            (set (reg/f:SI 7 sp) 
                (plus:SI (reg/f:SI 7 sp) 
                    (const_int -16 [0xfffffffffffffff0]))) 
            (clobber (reg:CC 17 flags)) 
        ]) 199 {*addsi_1} (nil) 
    (nil)) 
 
(note 7 15 22 0 NOTE_INSN_FUNCTION_BEG) 
 
(insn 22 7 73 0 (set (reg/v:SI 59 [ lsm_tmp1 ]) 
        (reg/v:SI 61 [ i ])) 42 {*movsi_1_nointernunit} (nil) 
    (nil)) 
 
(note 73 22 78 0 NOTE_INSN_LOOP_BEG) 
 
(insn 78 73 84 0 (set (reg:SI 75) 
        (mem/u/c:SI (plus:SI (mult:SI (reg/v:SI 61 [ i ]) 
                    (const_int 4 [0x4])) 
                (label_ref:SI 32)) [0 S4 A8])) 42 {*movsi_1_nointernunit} 
(nil) 
    (insn_list:REG_LABEL 32 (nil))) 
 
(jump_insn 84 78 85 0 (set (pc) 
        (label_ref 83)) -1 (nil) 
    (nil)) 
 
(barrier 85 84 70) 
 
(code_label 70 85 69 1 9 "" [3 uses]) 
 
(note 69 70 83 1 [bb 1] NOTE_INSN_BASIC_BLOCK) 
 
(code_label 83 69 82 7 11 "" [1 uses]) 
 
(note 82 83 26 7 [bb 7] NOTE_INSN_BASIC_BLOCK) 
 
(insn 26 82 27 7 (set (reg:CC 17 flags) 
        (compare:CC (reg/v:SI 59 [ lsm_tmp1 ]) 
            (const_int 5 [0x5]))) 5 {*cmpsi_1_insn} (nil) 
    (nil)) 
 
(jump_insn 27 26 80 7 (set (pc) 
        (if_then_else (gtu (reg:CC 17 flags) 
                (const_int 0 [0x0])) 
            (label_ref 70) 
            (pc))) 486 {*jcc_1} (nil) 
    (expr_list:REG_BR_PROB (const_int 5000 [0x1388]) 
        (nil))) 
 
(code_label 80 27 66 2 10 "" [1 uses]) 
 
(note 66 80 30 2 [bb 2] NOTE_INSN_BASIC_BLOCK) 
 
(jump_insn 30 66 31 2 (parallel [ 
            (set (pc) 
                (reg:SI 75)) 
            (use (label_ref 32)) 
        ]) 503 {*tablejump_1} (nil) 
    (nil)) 
 
(barrier 31 30 32) 
 
(code_label 32 31 33 6 "" [2 uses]) 
 
(jump_insn 33 32 34 (addr_vec:SI [ 
            (label_ref:SI 35) 
            (label_ref:SI 35) 
            (label_ref/s:SI 46) 
            (label_ref/s:SI 46) 
            (label_ref:SI 70) 
            (label_ref:SI 40) 
        ]) -1 (nil) 
    (nil)) 
 
(barrier 34 33 35) 
 
(code_label 35 34 36 3 3 "" [2 uses]) 
 
(note 36 35 37 3 [bb 3] NOTE_INSN_BASIC_BLOCK) 
 
(insn 37 36 38 3 (set (reg/v:SI 58 [ D1127 ]) 
        (const_int 1 [0x1])) 42 {*movsi_1_nointernunit} (nil) 
    (nil)) 
 
(jump_insn 38 37 39 3 (set (pc) 
        (label_ref/s 50)) -1 (nil) 
    (nil)) 
 
(barrier 39 38 40) 
 
(code_label 40 39 41 4 5 "" [1 uses]) 
 
(note 41 40 42 4 [bb 4] NOTE_INSN_BASIC_BLOCK) 
 
(insn 42 41 79 4 (set (reg/v:SI 59 [ lsm_tmp1 ]) 
        (const_int 4 [0x4])) 42 {*movsi_1_nointernunit} (nil) 
    (nil)) 
 
(insn 79 42 81 4 (set (reg:SI 75) 
        (label_ref:SI 70)) 42 {*movsi_1_nointernunit} (nil) 
    (insn_list:REG_LABEL 70 (nil))) 
 
(insn 81 79 43 4 (set (reg:CC 17 flags) 
        (compare:CC (reg/v:SI 59 [ lsm_tmp1 ]) 
            (const_int 5 [0x5]))) -1 (nil) 
    (nil)) 
 
(jump_insn 43 81 44 4 (set (pc) 
        (label_ref 80)) 500 {jump} (nil) 
    (nil)) 
 
(barrier 44 43 74) 
 
(note 74 44 46 NOTE_INSN_LOOP_END) 
 
(code_label 46 74 47 5 4 ("L3") [2 uses]) 
 
(note 47 46 48 5 [bb 5] NOTE_INSN_BASIC_BLOCK) 
 
(insn 48 47 50 5 (set (reg/v:SI 58 [ D1127 ]) 
        (const_int 0 [0x0])) 42 {*movsi_1_nointernunit} (nil) 
    (nil)) 
 
(code_label 50 48 51 6 7 ("L7") [1 uses]) 
 
(note 51 50 55 6 [bb 6] NOTE_INSN_BASIC_BLOCK) 
 
(note 55 51 58 6 NOTE_INSN_FUNCTION_END) 
 
(insn 58 55 64 6 (set (reg/i:SI 0 ax [ <result> ]) 
        (reg/v:SI 58 [ D1127 ])) 42 {*movsi_1_nointernunit} (nil) 
    (nil)) 
 
(insn 64 58 0 6 (use (reg/i:SI 0 ax [ <result> ])) -1 (nil) 
    (nil)) 
 
$15 = void 
 
 
The code_label that disappears is: 
 
(gdb) p debug_rtx_find (get_insns (), 70) 
(code_label 70 85 69 1 9 "" [3 uses]) 
 
The three references to the label are: 
 
(jump_insn 27 26 80 7 (set (pc) 
        (if_then_else (gtu (reg:CC 17 flags) 
                (const_int 0 [0x0])) 
            (label_ref 70) 
            (pc))) 486 {*jcc_1} (nil) 
    (expr_list:REG_BR_PROB (const_int 5000 [0x1388]) 
        (nil))) 
 
(jump_insn 33 32 34 (addr_vec:SI [ 
            (label_ref:SI 35) 
            (label_ref:SI 35) 
            (label_ref/s:SI 46) 
            (label_ref/s:SI 46) 
            (label_ref:SI 70) 
            (label_ref:SI 40) 
        ]) -1 (nil) 
    (nil)) 
 
(insn 79 42 81 4 (set (reg:SI 75) 
        (label_ref:SI 70)) 42 {*movsi_1_nointernunit} (nil) 
    (insn_list:REG_LABEL 70 (nil))) 
 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-12-15 14:11 -------
(In reply to comment #14)
> Given Roger's analysis, this bug is probably present in earlier GCCs as 
> well, so this is not really a regression. 

It is still an user visible regression aka it was a latent bug but now it shows up with more code than 
before because of tree-ssa.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-12-15 23:52 -------
Anyway, this is surely a low-priority regression, since the combination of 
flags needed to trigger this is unusual at least.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[micis at gmx dot de]

------- Additional Comments From micis at gmx dot de  2004-12-16 07:51 -------
Maybe you missed the example given in comment #3 which miscompiles with just "-
O2" 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[steven at gcc dot gnu dot org]

------- Additional Comments From steven at gcc dot gnu dot org  2005-01-05 17:08 -------
Any chance this is related to 18861? 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[reichelt at gcc dot gnu dot org]

------- Additional Comments From reichelt at gcc dot gnu dot org  2005-01-07 10:24 -------
Alas this is not fixed by the patch for PR18861.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0 regression] miscompilation of switch statement in loop

[neroden at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P2                          |P1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[aoliva at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |aoliva at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2005-02-26 18:42:03         |2005-03-09 10:19:05
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-09 10:30 -------
Subject: [PR middle-end/18628] do not fold to label load from tablejump to reg

This patch is meant to implement suggestion #3 proposed to fix the bug
by Roger Sayle and selected by RTH in bugzilla.  So far, I've only
verified that it fixes the testcase included in the patch.

The thought of adding the REG_EQUAL note was to help other passes that
might want to turn the indirect jump into a direct jump.  I'm not sure
this may actually happen.

Bootstrap and regtesting starting shortly.  Ok to install if they
pass?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/18628
	* cse.c (fold_rtx_mem): Instead of returning the label extracted
	from a tablejump, add it as an REG_EQUAL note, if the insn loaded
	from the table to a register.
	(cse_insn): Don't use it as src_eqv.

Index: gcc/cse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cse.c,v
retrieving revision 1.349
diff -u -p -r1.349 cse.c
--- gcc/cse.c 8 Mar 2005 13:56:56 -0000 1.349
+++ gcc/cse.c 9 Mar 2005 10:19:13 -0000
@@ -3515,8 +3515,36 @@ fold_rtx_mem (rtx x, rtx insn)
 	    if (offset >= 0
 		&& (offset / GET_MODE_SIZE (GET_MODE (table))
 		    < XVECLEN (table, 0)))
-	      return XVECEXP (table, 0,
-			      offset / GET_MODE_SIZE (GET_MODE (table)));
+	      {
+		rtx label = XVECEXP
+		  (table, 0, offset / GET_MODE_SIZE (GET_MODE (table)));
+		rtx set;
+
+		/* If we have an insn that loads the label from the
+		   jumptable into a reg, we don't want to set the reg
+		   to the label, because this may cause a reference to
+		   the label to remain after the label is removed in
+		   some very obscure cases (PR middle-end/18628).  So
+		   we just set a REG_EQUAL note for this case, and
+		   return the original MEM.  */
+		if (!insn)
+		  return label;
+
+		set = single_set (insn);		
+
+		if (! set || SET_SRC (set) != x)
+		  return x;
+
+		/* If it's a jump, it's safe to reference the label.  */
+		if (SET_DEST (set) == pc_rtx)
+		  return label;
+
+		/* If it's not a REG, the REG_EQUAL note is inappropriate.  */
+		if (REG_P (SET_DEST (set)))
+		  set_unique_reg_note (insn, REG_EQUAL, label);
+
+		return x;
+	      }
 	  }
 	if (table_insn && JUMP_P (table_insn)
 	    && GET_CODE (PATTERN (table_insn)) == ADDR_DIFF_VEC)
@@ -4861,6 +4889,13 @@ cse_insn (rtx insn, rtx libcall_insn)
     {
       src_eqv = fold_rtx (canon_reg (XEXP (tem, 0), NULL_RTX), insn);
       XEXP (tem, 0) = src_eqv;
+
+      /* We don't want to use the labels in REG_EQUAL notes that
+	 fold_rtx may have added in an earlier pass.  If it's
+	 something as simple as a LABEL_REF and we didn't set to it
+	 directly, there was a reason not to do so.  */
+      if (GET_CODE (src_eqv) == LABEL_REF)
+	src_eqv = NULL;
     }
 
   /* Canonicalize sources and addresses of destinations.
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* gcc.dg/pr18628.c: New.

Index: gcc/testsuite/gcc.dg/pr18628.c
===================================================================
RCS file: gcc/testsuite/gcc.dg/pr18628.c
diff -N gcc/testsuite/gcc.dg/pr18628.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/gcc.dg/pr18628.c 9 Mar 2005 10:19:27 -0000
@@ -0,0 +1,31 @@
+/* { dg-do link } */
+/* { dg-options "-O2" } */
+
+/* PR middle-end/18628 exposed a problem in which cse folded a load
+   from a jump table into the label that was the target of the branch.
+   Unfortunately, the indirect jump was moved to a different basic
+   block, and the LABEL_REF copied to the register wasn't enough to
+   keep the cfg from optimizing the otherwise-unused label away.  So
+   we ended up with a dangling reference to the label.  */
+
+int i;
+
+int main()
+{
+  for (;;)
+  {
+    switch (i)
+    {
+      case 0:
+      case 1:
+        return 1;
+
+      case 2:
+      case 3:
+        return 0;
+
+      case 5:
+        --i;
+    }
+  }
+}

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug middle-end/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[roger at eyesopen dot com]

------- Additional Comments From roger at eyesopen dot com  2005-03-09 16:13 -------
Subject: Re: [PR middle-end/18628] do not fold to label load from tablejump
 to reg


On 9 Mar 2005, Alexandre Oliva wrote:
> This patch is meant to implement suggestion #3 proposed to fix the bug
> by Roger Sayle and selected by RTH in bugzilla.  So far, I've only
> verified that it fixes the testcase included in the patch.
>
> Alexandre Oliva  <aoliva@redhat.com>
>
>       PR middle-end/18628
>       * cse.c (fold_rtx_mem): Instead of returning the label extracted
>       from a tablejump, add it as an REG_EQUAL note, if the insn loaded
>       from the table to a register.
>       (cse_insn): Don't use it as src_eqv.

Thanks!  OK for mainline if bootstrap and regression testing passes.
Once this patch has been on mainline for a few days, to check that targets
with different forms of tablejump and conditional branches don't have
issues, OK to backport to the 4.0 branch.  Thanks also to RTH for
selecting which of the proposals in the bugzilla PR he preferred.  I'll
admit that I hadn't noticed he'd commented on them until you'd posted
this patch.

However, full credit goes to your patch.  I hadn't appreciated that the
problematic transformation takes place in fold_rtx_mem which has the
instruction context, allowing us to perform this transformation when its
safe (i.e. we directly converting a tablejump into an unconditional jump)
but to avoid the problematic case of hositing a label_ref into a register
that can then escape.  Cool.


> The thought of adding the REG_EQUAL note was to help other passes that
> might want to turn the indirect jump into a direct jump.  I'm not sure
> this may actually happen.

I'm also not sure how much this will help.  If you do encounter any
problems with your patch, my first instinct would be to investigate
not bothering with the REG_EQUAL note.  We've had issues in the past
with whether label_refs in REG_EQUAL notes are counted in the label's
NUSES, and similar ugly corner cases.  If there's no measurable
performance impact, we're probably better off without the risk [on
the 4.0 branch at least].



> Bootstrap and regtesting starting shortly.  Ok to install if they
> pass?

Please forgive me for commenting on this, but it's kind of a pet peeve.
There really are no patches so urgent that they can't be bootstrapped
and regression tested before posting to gcc-patches.  Even "obvious"
fixes to target-independent bootstrap failures can affort the few hours
it takes to confirm changes work and are safe.  Indeed the language
(and emphasis) in contribute.html is (are) quite clear on the matter.
My apologies for bringing this up now as you're certainly not amongst
the worst offenders in this regard.

Many thanks again for tackling high-priority PR middle-end/18628.

Roger
--



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[rth at gcc dot gnu dot org]

------- Additional Comments From rth at gcc dot gnu dot org  2005-03-10 13:46 -------
Subject: Re: [PR middle-end/18628] do not fold to label load from tablejump to reg

On Wed, Mar 09, 2005 at 07:26:37AM -0300, Alexandre Oliva wrote:
> +		/* If it's not a REG, the REG_EQUAL note is inappropriate.  */
> +		if (REG_P (SET_DEST (set)))
> +		  set_unique_reg_note (insn, REG_EQUAL, label);

I don't think this is a good idea at all.  This is just
asking for reload to recreate a reference to the deleted label.


r~


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[aoliva at redhat dot com]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-10 20:38 -------
Subject: Re: [PR middle-end/18628] do not fold to label load from tablejump to reg

On Mar 10, 2005, Richard Henderson <rth@gcc.gnu.org> wrote:

> On Wed, Mar 09, 2005 at 07:26:37AM -0300, Alexandre Oliva wrote:
>> +		/* If it's not a REG, the REG_EQUAL note is inappropriate.  */
>> +		if (REG_P (SET_DEST (set)))
>> +		  set_unique_reg_note (insn, REG_EQUAL, label);

> I don't think this is a good idea at all.  This is just
> asking for reload to recreate a reference to the deleted label.

Here's a patch with that bit removed, along with the change in
cse_init that it required.  Ok?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/18628
	* cse.c (fold_rtx_mem): Don't fold a load from a jumptable into a
	register.

Index: gcc/cse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cse.c,v
retrieving revision 1.349
diff -u -p -r1.349 cse.c
--- gcc/cse.c 8 Mar 2005 13:56:56 -0000 1.349
+++ gcc/cse.c 10 Mar 2005 20:36:36 -0000
@@ -3515,8 +3515,30 @@ fold_rtx_mem (rtx x, rtx insn)
 	    if (offset >= 0
 		&& (offset / GET_MODE_SIZE (GET_MODE (table))
 		    < XVECLEN (table, 0)))
-	      return XVECEXP (table, 0,
-			      offset / GET_MODE_SIZE (GET_MODE (table)));
+	      {
+		rtx label = XVECEXP
+		  (table, 0, offset / GET_MODE_SIZE (GET_MODE (table)));
+		rtx set;
+
+		/* If we have an insn that loads the label from the
+		   jumptable into a reg, we don't want to set the reg
+		   to the label, because this may cause a reference to
+		   the label to remain after the label is removed in
+		   some very obscure cases (PR middle-end/18628).  */
+		if (!insn)
+		  return label;
+
+		set = single_set (insn);		
+
+		if (! set || SET_SRC (set) != x)
+		  return x;
+
+		/* If it's a jump, it's safe to reference the label.  */
+		if (SET_DEST (set) == pc_rtx)
+		  return label;
+
+		return x;
+	      }
 	  }
 	if (table_insn && JUMP_P (table_insn)
 	    && GET_CODE (PATTERN (table_insn)) == ADDR_DIFF_VEC)
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* gcc.dg/pr18628.c: New.

Index: gcc/testsuite/gcc.dg/pr18628.c
===================================================================
RCS file: gcc/testsuite/gcc.dg/pr18628.c
diff -N gcc/testsuite/gcc.dg/pr18628.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/gcc.dg/pr18628.c 10 Mar 2005 20:36:52 -0000
@@ -0,0 +1,31 @@
+/* { dg-do link } */
+/* { dg-options "-O2" } */
+
+/* PR middle-end/18628 exposed a problem in which cse folded a load
+   from a jump table into the label that was the target of the branch.
+   Unfortunately, the indirect jump was moved to a different basic
+   block, and the LABEL_REF copied to the register wasn't enough to
+   keep the cfg from optimizing the otherwise-unused label away.  So
+   we ended up with a dangling reference to the label.  */
+
+int i;
+
+int main()
+{
+  for (;;)
+  {
+    switch (i)
+    {
+      case 0:
+      case 1:
+        return 1;
+
+      case 2:
+      case 3:
+        return 0;
+
+      case 5:
+        --i;
+    }
+  }
+}

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[pluto at pld-linux dot org]

------- Additional Comments From pluto at pld-linux dot org  2005-03-10 23:00 -------
I've just built qt334/kde340(core) with -march=i686 -O/-O2. 
They are totally broken (damaged grahpics/widgets) :/ 
I don't have a testcase at this moment but gcc4 has a serious bug. 
 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2005-03-10 23:13 -------
(In reply to comment #27)
> I've just built qt334/kde340(core) with -march=i686 -O/-O2. 
> They are totally broken (damaged grahpics/widgets) :/ 
> I don't have a testcase at this moment but gcc4 has a serious bug. 
That is most likely PR 19317.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[law at redhat dot com]

------- Additional Comments From law at redhat dot com  2005-03-11 00:51 -------
Subject: Re: [PR middle-end/18628] do not fold to label load from tablejump
	to reg

On Thu, 2005-03-10 at 17:37 -0300, Alexandre Oliva wrote:
> On Mar 10, 2005, Richard Henderson <rth@gcc.gnu.org> wrote:
> 
> > On Wed, Mar 09, 2005 at 07:26:37AM -0300, Alexandre Oliva wrote:
> >> +		/* If it's not a REG, the REG_EQUAL note is inappropriate.  */
> >> +		if (REG_P (SET_DEST (set)))
> >> +		  set_unique_reg_note (insn, REG_EQUAL, label);
> 
> > I don't think this is a good idea at all.  This is just
> > asking for reload to recreate a reference to the deleted label.
> 
> Here's a patch with that bit removed, along with the change in
> cse_init that it required.  Ok?
BTW, can you open a new bug with the same testcase and attach it to the
"things CSE still catches" meta-bug.  I'd like to catch this at the
tree level if we can rather than wait for our friend CSE.

jeff



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[rth at gcc dot gnu dot org]

------- Additional Comments From rth at gcc dot gnu dot org  2005-03-11 12:45 -------
Subject: Re: [PR middle-end/18628] do not fold to label load from tablejump to reg

On Thu, Mar 10, 2005 at 05:37:48PM -0300, Alexandre Oliva wrote:
> 	* cse.c (fold_rtx_mem): Don't fold a load from a jumptable into a
> 	register.

Ok.


r~


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2005-03-14 20:06 -------
Subject: Bug 18628

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	aoliva@gcc.gnu.org	2005-03-14 20:06:23

Modified files:
	gcc            : ChangeLog cse.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: pr18628.c 

Log message:
	gcc/ChangeLog:
	PR middle-end/18628
	* cse.c (fold_rtx_mem): Don't fold a load from a jumptable into a
	register.
	gcc/testsuite/ChangeLog:
	* gcc.dg/pr18628.c: New.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.7849&r2=2.7850
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cse.c.diff?cvsroot=gcc&r1=1.349&r2=1.350
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.5166&r2=1.5167
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/pr18628.c.diff?cvsroot=gcc&r1=NONE&r2=1.1



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2005-03-14 20:32 -------
Subject: Bug 18628

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	aoliva@gcc.gnu.org	2005-03-14 20:32:50

Modified files:
	gcc            : ChangeLog cse.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: pr18628.c 

Log message:
	gcc/ChangeLog:
	PR middle-end/18628
	* cse.c (fold_rtx_mem): Don't fold a load from a jumptable into a
	register.
	gcc/testsuite/ChangeLog:
	* gcc.dg/pr18628.c: New.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.49&r2=2.7592.2.50
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cse.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.345&r2=1.345.2.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5084.2.44&r2=1.5084.2.45
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/pr18628.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628

[Bug rtl-optimization/18628] [4.0/4.1 regression] miscompilation of switch statement in loop

[aoliva at gcc dot gnu dot org]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-03-14 23:43 -------
Fixed

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18628