Re: [Committed] S/390: PR57609 fix - use next_active_insn instead of next_real_insn

2013-06-19 Thread Andreas Krebbel 
On 18/06/13 19:06, Steven Bosscher wrote:
 BTW I don't understand how a label satisfying the following can be
 true for a label before a jump table:
 
   if (LABEL_P (insn)
(LABEL_PRESERVE_P (insn) || LABEL_NAME (insn)))
 
 LABEL_PRESERVE_P should never be set on a label before a
 JUMP_TABLE_DATA, and LABEL_NAME should be NULL.

Actually LABEL_PRESERVE_P appears to be set on quite many of the jump table 
data labels.  Example
from compiling fold-const.c:

(code_label/s 1285 1284 1286 9315  [3 uses])

(jump_table_data 1286 1285 1287 (addr_vec:SI [
(label_ref:SI 54875)
(label_ref:SI 63283)
(label_ref:SI 63283)
(label_ref:SI 63283)
(label_ref:SI 63283)
(label_ref:SI 63283)
(label_ref:SI 63283) ...

Hardware watchpoint 5: table_label-in_struct

Old value = 0
New value = 1
force_const_mem (mode=SImode, x=0x7c0c27c0) at /build/gcc-head/gcc/varasm.c:3699
3699  return copy_rtx (def);
(gdb) bt
#0  force_const_mem (mode=SImode, x=0x7c0c27c0) at 
/build/gcc-head/gcc/varasm.c:3699
#1  0x009ada54 in emit_move_insn (x=0x7bdf44b0, y=0x7c0c27c0) at 
/build/gcc-head/gcc/expr.c:3499
#2  0x010b3e6a in gen_casesi (operand0=0x7bdf42d0, operand1=0x7d7e24e0, 
operand2=0x7c0c20e0,
operand3=0x7b3c4488, operand4=0x7b3c43c0) at 
/build/gcc-head/gcc/config/s390/s390.md:8588
#3  0x00c0c70e in maybe_gen_insn (icode=CODE_FOR_casesi, nops=5, ops=0x7fffe3a8)
at /build/gcc-head/gcc/optabs.c:8219
#4  0x00c0c92a in maybe_expand_jump_insn (icode=CODE_FOR_casesi, nops=5, 
ops=0x7fffe3a8)
at /build/gcc-head/gcc/optabs.c:8257
#5  0x00c0c9ee in expand_jump_insn (icode=CODE_FOR_casesi, nops=5, 
ops=0x7fffe3a8)
at /build/gcc-head/gcc/optabs.c:8283
#6  0x009ca5ec in try_casesi (index_type=0x7d7e6420, index_expr=0x7bca4168, 
minval=0x7d889300,
range=0x7d156920, table_label=0x7b3c4488, default_label=0x7b3c43c0, 
fallback_label=0x7b3c43e8,
default_probability=) at /build/gcc-head/gcc/expr.c:10967
#7  0x00d18016 in emit_case_dispatch_table (index_expr=0x7bca4168, 
index_type=0x7d7e6420,
case_list=0x1a31d58, default_label=0x7b3c43c0, minval=0x7d889300, 
maxval=0x7d0ad180,
range=0x7d156920, stmt_bb=0x7bf96000) at /build/gcc-head/gcc/stmt.c:1933
#8  0x00d18ef4 in expand_case (stmt=0x7d7dc800) at 
/build/gcc-head/gcc/stmt.c:2207

 Better yet would be to use tablejump_p instead of examining the
 pattern by hand, e.g.:

Ok. Better. I've applied your patch after testing it. Thanks!

Bye,

-Andreas-

 
 Index: s390.c
 ===
 --- s390.c  (revision 200173)
 +++ s390.c  (working copy)
 @@ -7023,7 +7023,7 @@ s390_chunkify_start (void)
if (LABEL_P (insn)
(LABEL_PRESERVE_P (insn) || LABEL_NAME (insn)))
 {
 - rtx vec_insn = next_active_insn (insn);
 + rtx vec_insn = NEXT_INSN (insn);
   if (! vec_insn || ! JUMP_TABLE_DATA_P (vec_insn))
 bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (insn));
 }
 @@ -7033,6 +7033,8 @@ s390_chunkify_start (void)
else if (JUMP_P (insn))
 {
rtx pat = PATTERN (insn);
 +  rtx table;
 +
   if (GET_CODE (pat) == PARALLEL  XVECLEN (pat, 0)  2)
 pat = XVECEXP (pat, 0, 0);
 
 @@ -7046,28 +7048,18 @@ s390_chunkify_start (void)
 bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (label));
 }
  }
 - else if (GET_CODE (pat) == PARALLEL
 -   XVECLEN (pat, 0) == 2
 -   GET_CODE (XVECEXP (pat, 0, 0)) == SET
 -   GET_CODE (XVECEXP (pat, 0, 1)) == USE
 -   GET_CODE (XEXP (XVECEXP (pat, 0, 1), 0)) == LABEL_REF)
 -   {
 - /* Find the jump table used by this casesi jump.  */
 - rtx vec_label = XEXP (XEXP (XVECEXP (pat, 0, 1), 0), 0);
 - rtx vec_insn = next_active_insn (vec_label);
 - if (vec_insn  JUMP_TABLE_DATA_P (vec_insn))
 -   {
 - rtx vec_pat = PATTERN (vec_insn);
 - int i, diff_p = GET_CODE (vec_pat) == ADDR_DIFF_VEC;
 -
 - for (i = 0; i  XVECLEN (vec_pat, diff_p); i++)
 -   {
 - rtx label = XEXP (XVECEXP (vec_pat, diff_p, i), 0);
 -
 - if (s390_find_pool (pool_list, label)
 - != s390_find_pool (pool_list, insn))
 -   bitmap_set_bit (far_labels, CODE_LABEL_NUMBER 
 (label));
 -   }
 + else if (tablejump_p (insn, NULL, table))
 +   {
 + rtx vec_pat = PATTERN (table);
 + int i, diff_p = GET_CODE (vec_pat) == ADDR_DIFF_VEC;
 +
 + for (i = 0; i  XVECLEN (vec_pat, diff_p); i++)
 +   {
 + rtx label = XEXP (XVECEXP (vec_pat, diff_p, i), 0);
 +
 + if (s390_find_pool (pool_list, label)
 +

Re: [Committed] S/390: PR57609 fix - use next_active_insn instead of next_real_insn

2013-06-18 Thread Steven Bosscher 
On Tue, Jun 18, 2013 at 10:57 AM, Andreas Krebbel
kreb...@linux.vnet.ibm.com wrote:
 Hi,

 the patch replaces next_real_insn with next_active_insn when checking
 for JUMP TABLE insns.

 This fixes ESA mode bootstrap on s390 which broke with r197266.

 Comitted to mainline

Please revert this and find another solution. JUMP_TABLE_DATA is not
an active insn, and I will be removing it soon from active_insn_p.

How big does a FIXME have to be for people to notice?

Ciao!
Steven

Re: [Committed] S/390: PR57609 fix - use next_active_insn instead of next_real_insn

2013-06-18 Thread Chung-Ju Wu 
2013/6/18 Steven Bosscher stevenb@gmail.com:
 On Tue, Jun 18, 2013 at 10:57 AM, Andreas Krebbel
 kreb...@linux.vnet.ibm.com wrote:
 Hi,

 the patch replaces next_real_insn with next_active_insn when checking
 for JUMP TABLE insns.

 This fixes ESA mode bootstrap on s390 which broke with r197266.

 Comitted to mainline

 Please revert this and find another solution. JUMP_TABLE_DATA is not
 an active insn, and I will be removing it soon from active_insn_p.

 How big does a FIXME have to be for people to notice?

 Ciao!
 Steven

Hi, Steven,

Do you mean that your comment 6 in PR56809 is not a good solution
since you are going to remove JUMP_TABLE_DAT from active_insn_p ??

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



Best regards,
jasonwucj

Re: [Committed] S/390: PR57609 fix - use next_active_insn instead of next_real_insn

2013-06-18 Thread Steven Bosscher 
On Tue, Jun 18, 2013 at 11:29 AM, Chung-Ju Wu wrote:
 Do you mean that your comment 6 in PR56809 is not a good solution
 since you are going to remove JUMP_TABLE_DAT from active_insn_p ??

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

No, that one I'll fix, it's fall-out from the JUMP_TABLE_DATA change
and thus my responsibility to fix.

Ciao!
Steven

Re: [Committed] S/390: PR57609 fix - use next_active_insn instead of next_real_insn

2013-06-18 Thread Andreas Krebbel 
On 18/06/13 11:35, Steven Bosscher wrote:
 On Tue, Jun 18, 2013 at 11:29 AM, Chung-Ju Wu wrote:
 Do you mean that your comment 6 in PR56809 is not a good solution
 since you are going to remove JUMP_TABLE_DAT from active_insn_p ??

 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56809
 
 No, that one I'll fix, it's fall-out from the JUMP_TABLE_DATA change
 and thus my responsibility to fix.

Btw. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57609 is also a fall-out from 
the JUMP_TABLE_DATA
change.

-Andreas-

Re: [Committed] S/390: PR57609 fix - use next_active_insn instead of next_real_insn

2013-06-18 Thread Andreas Krebbel 
On Tue, Jun 18, 2013 at 10:59:56AM +0200, Steven Bosscher wrote:
 On Tue, Jun 18, 2013 at 10:57 AM, Andreas Krebbel
 kreb...@linux.vnet.ibm.com wrote:
  Hi,
 
  the patch replaces next_real_insn with next_active_insn when checking
  for JUMP TABLE insns.
 
  This fixes ESA mode bootstrap on s390 which broke with r197266.
 
  Comitted to mainline
 
 Please revert this and find another solution. JUMP_TABLE_DATA is not
 an active insn, and I will be removing it soon from active_insn_p.

I don't see which of the other iterators would fit here.  So I'll need
my own.  How do you intend to fix this for the other targets?  Will
there be a new iterator available?

If you don't want me to use next_active_insn I probably have to do
something like this instead:

---
 gcc/config/s390/s390.c |   30 ++
 1 file changed, 22 insertions(+), 8 modifications(!)

Index: gcc/config/s390/s390.c
===
*** gcc/config/s390/s390.c.orig
--- gcc/config/s390/s390.c
*** s390_mainpool_cancel (struct constant_po
*** 6792,6797 
--- 6792,6819 
s390_free_pool (pool);
  }
  
+ /* Return true if LABEL is directly followed by a jump table insn.  If
+JUMP_TABLE_INSN is non-null the jump table insn is returned
+there.  */
+ static bool
+ s390_is_jumptable_label_p (rtx label, rtx *jump_table_insn)
+ {
+   while (label)
+ {
+   label = NEXT_INSN (label);
+ 
+   if (label == NULL_RTX || NONDEBUG_INSN_P (label))
+   return false;
+ 
+   if (JUMP_TABLE_DATA_P (label))
+   {
+ if (jump_table_insn != NULL)
+   *jump_table_insn = label;
+ return true;
+   }
+ }
+   return false;
+ }
  
  /* Chunkify the literal pool.  */
  
*** s390_chunkify_start (void)
*** 7012,7019 
if (LABEL_P (insn)
   (LABEL_PRESERVE_P (insn) || LABEL_NAME (insn)))
{
! rtx vec_insn = next_active_insn (insn);
! if (! vec_insn || ! JUMP_TABLE_DATA_P (vec_insn))
bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (insn));
}
  
--- 7034,7040 
if (LABEL_P (insn)
   (LABEL_PRESERVE_P (insn) || LABEL_NAME (insn)))
{
! if (!s390_is_jumptable_label_p (insn, NULL))
bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (insn));
}
  
*** s390_chunkify_start (void)
*** 7043,7050 
{
  /* Find the jump table used by this casesi jump.  */
  rtx vec_label = XEXP (XEXP (XVECEXP (pat, 0, 1), 0), 0);
! rtx vec_insn = next_active_insn (vec_label);
! if (vec_insn  JUMP_TABLE_DATA_P (vec_insn))
{
  rtx vec_pat = PATTERN (vec_insn);
  int i, diff_p = GET_CODE (vec_pat) == ADDR_DIFF_VEC;
--- 7064,7072 
{
  /* Find the jump table used by this casesi jump.  */
  rtx vec_label = XEXP (XEXP (XVECEXP (pat, 0, 1), 0), 0);
! rtx vec_insn;
! 
! if (s390_is_jumptable_label_p (vec_label, vec_insn))
{
  rtx vec_pat = PATTERN (vec_insn);
  int i, diff_p = GET_CODE (vec_pat) == ADDR_DIFF_VEC;

Re: [Committed] S/390: PR57609 fix - use next_active_insn instead of next_real_insn

2013-06-18 Thread Steven Bosscher 
On Tue, Jun 18, 2013 at 2:17 PM, Andreas Krebbel wrote:
 If you don't want me to use next_active_insn I probably have to do
 something like this instead:

No. If the label is followed by jump table data, then NEXT_INSN(label)
will be the JUMP_TABLE_DATA rtx. See tablejump_p.

So the following should work:

Index: s390.c
===
--- s390.c  (revision 200173)
+++ s390.c  (working copy)
@@ -7023,7 +7023,7 @@ s390_chunkify_start (void)
   if (LABEL_P (insn)
   (LABEL_PRESERVE_P (insn) || LABEL_NAME (insn)))
{
- rtx vec_insn = next_active_insn (insn);
+ rtx vec_insn = NEXT_INSN (insn);
  if (! vec_insn || ! JUMP_TABLE_DATA_P (vec_insn))
bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (insn));
}
@@ -7054,7 +7054,7 @@ s390_chunkify_start (void)
{
  /* Find the jump table used by this casesi jump.  */
  rtx vec_label = XEXP (XEXP (XVECEXP (pat, 0, 1), 0), 0);
- rtx vec_insn = next_active_insn (vec_label);
+ rtx vec_insn = NEXT_INSN (vec_label);
  if (vec_insn  JUMP_TABLE_DATA_P (vec_insn))
{
  rtx vec_pat = PATTERN (vec_insn);


BTW I don't understand how a label satisfying the following can be
true for a label before a jump table:

  if (LABEL_P (insn)
   (LABEL_PRESERVE_P (insn) || LABEL_NAME (insn)))

LABEL_PRESERVE_P should never be set on a label before a
JUMP_TABLE_DATA, and LABEL_NAME should be NULL.

Better yet would be to use tablejump_p instead of examining the
pattern by hand, e.g.:

Index: s390.c
===
--- s390.c  (revision 200173)
+++ s390.c  (working copy)
@@ -7023,7 +7023,7 @@ s390_chunkify_start (void)
   if (LABEL_P (insn)
   (LABEL_PRESERVE_P (insn) || LABEL_NAME (insn)))
{
- rtx vec_insn = next_active_insn (insn);
+ rtx vec_insn = NEXT_INSN (insn);
  if (! vec_insn || ! JUMP_TABLE_DATA_P (vec_insn))
bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (insn));
}
@@ -7033,6 +7033,8 @@ s390_chunkify_start (void)
   else if (JUMP_P (insn))
{
   rtx pat = PATTERN (insn);
+  rtx table;
+
  if (GET_CODE (pat) == PARALLEL  XVECLEN (pat, 0)  2)
pat = XVECEXP (pat, 0, 0);

@@ -7046,28 +7048,18 @@ s390_chunkify_start (void)
bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (label));
}
 }
- else if (GET_CODE (pat) == PARALLEL
-   XVECLEN (pat, 0) == 2
-   GET_CODE (XVECEXP (pat, 0, 0)) == SET
-   GET_CODE (XVECEXP (pat, 0, 1)) == USE
-   GET_CODE (XEXP (XVECEXP (pat, 0, 1), 0)) == LABEL_REF)
-   {
- /* Find the jump table used by this casesi jump.  */
- rtx vec_label = XEXP (XEXP (XVECEXP (pat, 0, 1), 0), 0);
- rtx vec_insn = next_active_insn (vec_label);
- if (vec_insn  JUMP_TABLE_DATA_P (vec_insn))
-   {
- rtx vec_pat = PATTERN (vec_insn);
- int i, diff_p = GET_CODE (vec_pat) == ADDR_DIFF_VEC;
-
- for (i = 0; i  XVECLEN (vec_pat, diff_p); i++)
-   {
- rtx label = XEXP (XVECEXP (vec_pat, diff_p, i), 0);
-
- if (s390_find_pool (pool_list, label)
- != s390_find_pool (pool_list, insn))
-   bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (label));
-   }
+ else if (tablejump_p (insn, NULL, table))
+   {
+ rtx vec_pat = PATTERN (table);
+ int i, diff_p = GET_CODE (vec_pat) == ADDR_DIFF_VEC;
+
+ for (i = 0; i  XVECLEN (vec_pat, diff_p); i++)
+   {
+ rtx label = XEXP (XVECEXP (vec_pat, diff_p, i), 0);
+
+ if (s390_find_pool (pool_list, label)
+ != s390_find_pool (pool_list, insn))
+   bitmap_set_bit (far_labels, CODE_LABEL_NUMBER (label));
}
}
 }

Ciao!
Steven