This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[4.0 PATCH] Fix PR 20917 (glibc build failure on s390)


Hello Mark,

PR 20917 is an ICE when building glibc with GCC 4.0 on s390, 
a serious regression.  It is apparently caused by a latent
bug in the way regmove handles REG_DEAD notes, which was 
exposed by changes I made to the __builtin_set_thread_pointer
implementation on s390.

At this point, I don't want to suggest any changes to regmove
for 4.0.  However, the patch below works around the problem by
back-end changes to the __builtin_set_thread_pointer patterns
that prevent the problematic code in regmove from being triggered.

Can I commit this patch for GCC 4.0?  It should be quite safe,
it doesn't change anything on non-s390 platforms, and on s390
it affects solely callers of __builtin_set_thread_pointer
(which usually *is* only glibc).

More details on the problem:

The test case uses __builtin_set_thread_pointer to set the
thread pointer to a value, and later uses that value again
as address in a memory access:

(insn 25 23 26 2 (set (reg:SI 36 %a0 [ p ])
        (reg/v/f:SI 43 [ p ])) 47 {*movsi_esa} (nil)
    (nil))

(insn 26 25 31 2 (unspec_volatile [
            (reg:SI 36 %a0)
        ] 500) -1 (insn_list:REG_DEP_TRUE 25 (nil))
    (expr_list:REG_DEAD (reg:SI 36 %a0)
        (nil)))

(insn 34 31 40 2 (set (reg/i:SI 2 %r2 [ <result> ])
        (mem/f:SI (reg/v/f:SI 43 [ p ]) [3 S4 A32])) 47 {*movsi_esa} (nil)
    (expr_list:REG_DEAD (reg/v/f:SI 43 [ p ])
        (nil)))

Insn 26 was inserted by the back end to prevent the set of the
thread pointer register %a0 from being eliminated as a dead store.
As %a0 is not used further on, it is correctly marked as dead here.

Now, after regmove, the REG_NOTE note has been moved:

(insn 25 23 26 2 (set (reg:SI 36 %a0 [ p ])
        (reg/v/f:SI 43 [ p ])) 47 {*movsi_esa} (nil)
    (nil))

(insn 26 25 31 2 (unspec_volatile [
            (reg:SI 36 %a0)
        ] 500) 334 {*set_tp} (insn_list:REG_DEP_TRUE 25 (nil))
    (nil))

(insn 34 31 40 2 (set (reg/i:SI 2 %r2 [ <result> ])
        (mem/f:SI (reg/v/f:SI 43 [ p ]) [3 S4 A32])) 47 {*movsi_esa} (nil)
    (expr_list:REG_DEAD (reg:SI 36 %a0)
        (expr_list:REG_DEAD (reg/v/f:SI 43 [ p ])
            (nil))))

Now the note is definitely incorrect, as the register does
not die in this insn.  This causes confusion in the scheduler
as dead notes are recomputed, triggering the ICE.

The reason appears to be the handling of dead notes in
regmove.c:optimize_reg_copy_1.  The routine notices the
register-to-register move 25, and tries to eliminate reg 43
by replacing its use in insn 34 by register %a0.

This fails because the special register %a0 is not valid
for use as base register.  However, optimize_reg_copy_1
has already removed the dead note from insn 26 and attached
it to insn 34 instead (because %a0 would indeed no longer be
dead if %a0 were introduced in insn 34); it doesn't clean up
this move after the attempt to change insn 34 itself failed.

The patch below changes the marker insn 26 to use a SET
to register %a0.  This causes regmove to consider the
register as modified, and thus the above optimization
is not even attempted.

Bootstrapped/regtested on s390-ibm-linux and s390x-ibm-linux.
OK for 4.0 branch?

Thanks to Andreas Jaeger for reporting this problem.

Bye,
Ulrich


ChangeLog:

	PR middle-end/20917
	* config/s390/s390.md ("*set_tp"): Use SET in pattern.
	("set_tp_64", "set_tp_31"): Adapt expanded pattern.


Index: gcc/config/s390/s390.md
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/s390/s390.md,v
retrieving revision 1.154.2.1
diff -c -p -r1.154.2.1 s390.md
*** gcc/config/s390/s390.md	9 Mar 2005 22:07:47 -0000	1.154.2.1
--- gcc/config/s390/s390.md	9 Apr 2005 17:08:28 -0000
***************
*** 7780,7797 ****
  
  (define_expand "set_tp_64"
    [(set (reg:DI 36) (match_operand:DI 0 "nonimmediate_operand" ""))
!    (unspec_volatile [(reg:DI 36)] UNSPECV_SET_TP)]
    "TARGET_64BIT"
    "")
  
  (define_expand "set_tp_31"
    [(set (reg:SI 36) (match_operand:SI 0 "nonimmediate_operand" ""))
!    (unspec_volatile [(reg:SI 36)] UNSPECV_SET_TP)]
    "!TARGET_64BIT"
    "")
  
  (define_insn "*set_tp"
!   [(unspec_volatile [(reg 36)] UNSPECV_SET_TP)]
    ""
    ""
    [(set_attr "type" "none")
--- 7780,7797 ----
  
  (define_expand "set_tp_64"
    [(set (reg:DI 36) (match_operand:DI 0 "nonimmediate_operand" ""))
!    (set (reg:DI 36) (unspec_volatile:DI [(reg:DI 36)] UNSPECV_SET_TP))]
    "TARGET_64BIT"
    "")
  
  (define_expand "set_tp_31"
    [(set (reg:SI 36) (match_operand:SI 0 "nonimmediate_operand" ""))
!    (set (reg:SI 36) (unspec_volatile:SI [(reg:SI 36)] UNSPECV_SET_TP))]
    "!TARGET_64BIT"
    "")
  
  (define_insn "*set_tp"
!   [(set (reg 36) (unspec_volatile [(reg 36)] UNSPECV_SET_TP))]
    ""
    ""
    [(set_attr "type" "none")
*** /dev/null	Tue Oct 26 21:02:43 2004
--- gcc/testsuite/gcc.dg/20050409-1.c	Sat Apr  9 19:28:27 2005
***************
*** 0 ****
--- 1,18 ----
+ /* This used to ICE due to a regmove problem on s390.  */
+ 
+ /* { dg-do compile { target s390*-*-* } } */
+ /* { dg-options "-O2" } */
+ 
+ 
+ extern void abort (void);
+ extern void **alloc (void);
+ 
+ void *test (void)
+ {
+   void **p = alloc ();
+   if (!p) abort ();
+ 
+   __builtin_set_thread_pointer (p);
+   return *p;
+ }
+ 
-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]