[ARM] Use r9 or r10 as the PIC register only when TARGET_32BIT

[ARM] Use r9 or r10 as the PIC register only when TARGET_32BIT

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

I'm investigating

FAIL: gcc.target/arm/pr42495.c (internal compiler error)
FAIL: gcc.target/arm/pr42495.c (test for excess errors)
FAIL: gcc.target/arm/pr42495.c scan-rtl-dump hoist "PRE/HOIST: end of bb 
.* copying expression": dump file does not exist

for arm-uclinuxeabi target. The cause of the ICE is because GCC uses r9 
as the PIC register for thumb1 code. This patch should fix it.

The original line of the code is several years old. It might have been 
broken for a long time. It could be no one use -fpic on ARM uClinux or 
-msingle-pic-base on other arm targets. Or I missed something.

Does this patch look OK? This patch fixes the above FAILs, but I have 
not fully tested it. I will do the regression testing with some other 
patches in a few days.


Regards,
-- 
Jie Zhang
CodeSourcery

[-- Attachment #2: gcc-arm-fix-thumb1-pic.diff --]
[-- Type: text/x-patch, Size: 680 bytes --]


	* config/arm/arm.c (arm_option_override): Use r9 or r10
	as the PIC register only when TARGET_32BIT.

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 164646)
+++ config/arm/arm.c	(working copy)
@@ -1868,7 +1868,7 @@ arm_option_override (void)
 
   /* If stack checking is disabled, we can use r10 as the PIC register,
      which keeps r9 available.  The EABI specifies r9 as the PIC register.  */
-  if (flag_pic && TARGET_SINGLE_PIC_BASE)
+  if (flag_pic && TARGET_SINGLE_PIC_BASE && TARGET_32BIT)
     {
       if (TARGET_VXWORKS_RTP)
 	warning (0, "RTP PIC is incompatible with -msingle-pic-base");

Re: [ARM] Use r9 or r10 as the PIC register only when TARGET_32BIT

[Paul Brook]

> The original line of the code is several years old. It might have been
> broken for a long time. It could be no one use -fpic on ARM uClinux or
> -msingle-pic-base on other arm targets. Or I missed something.

AFAIK arm-uclinux only has a relatively small number of users and I'd bet most 
of those don't use PIC[1].  It's entirely possible you're the first person to 
notice and care that -mthumb -fPIC broke[2].  -msingle-pic-base is an ABI 
changing option, so specifying it manually (rather than inheriting your OS 
defaults) almost certainly means you're doing something else wrong.

> Does this patch look OK?

No.

The whole point of -msingle-pic-base is to use a fixed base register for data 
addressing (in EABI terms, this is SB-relative addressing).  The pic base 
register is specified by the ABI.  The fact that legacy targets can't decide 
whether to use r9 or r10 is probably broken, but I find it hard to care about 
such targets.

i.e. we really do want to use r9 as the pic base register.  You need to fix 
whatever broke, rather than unilaterally picking a more convenient register.

Paul

[1] One of the main motivations for PIC is shared libraries.  However arm-
uclinux shared library support is extremely limited and crufty, if it works at 
all.  Also, it is not possible to mix PIC and non-PIC code, so you either need 
to flip the default or build multilibs.
[2] Last time I checked this is just one of a long list of things that didn't 
work out of the box on arm-uclinux.

Re: [ARM] Use r9 or r10 as the PIC register only when TARGET_32BIT

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]

On 01/12/2011 08:52 PM, Paul Brook wrote:
>> The original line of the code is several years old. It might have been
>> broken for a long time. It could be no one use -fpic on ARM uClinux or
>> -msingle-pic-base on other arm targets. Or I missed something.
>
> AFAIK arm-uclinux only has a relatively small number of users and I'd bet most
> of those don't use PIC[1].  It's entirely possible you're the first person to
> notice and care that -mthumb -fPIC broke[2].  -msingle-pic-base is an ABI
> changing option, so specifying it manually (rather than inheriting your OS
> defaults) almost certainly means you're doing something else wrong.
>
>> Does this patch look OK?
>
> No.
>
> The whole point of -msingle-pic-base is to use a fixed base register for data
> addressing (in EABI terms, this is SB-relative addressing).  The pic base
> register is specified by the ABI.  The fact that legacy targets can't decide
> whether to use r9 or r10 is probably broken, but I find it hard to care about
> such targets.
>
> i.e. we really do want to use r9 as the pic base register.  You need to fix
> whatever broke, rather than unilaterally picking a more convenient register.
>
Thanks for review. I thought THUMB1 code could not use r9. Apparently I 
was wrong. So I took a closer look at this issue. This time I found it 
was a recent regression caused by r162595 several months ago. That 
revision changed ARM port to generate pic address by using 
calculate_pic_address pattern, but didn't change 
thumb1_legitimate_address_p to accept the new address for THUMB1. The 
second issue is the split pattern does not always generate valid address 
for THUMB1. This patch should fix both issues. Regression tested on our 
internal 4.5 branch for arm-uclinuxeabi for gcc, g++ and libstdc++. The 
following FAILs are fixed:

FAIL: gcc.target/arm/pr42495.c (internal compiler error)
FAIL: gcc.target/arm/pr42495.c (test for excess errors)
FAIL: gcc.target/arm/pr42495.c scan-rtl-dump hoist "PRE/HOIST: end of bb 
.* copying expression": dump file does not exist
FAIL: gcc.target/arm/pr42574.c (internal compiler error)
FAIL: gcc.target/arm/pr42574.c (test for excess errors)
ERROR: gcc.target/arm/pr42574.c: error executing dg-final: couldn't open 
"pr42574.s": no such file or directory
UNRESOLVED: gcc.target/arm/pr42574.c: error executing dg-final: couldn't 
open "pr42574.s": no such file or directory

I'm lack of infrastructure to test it against FSF trunk.

OK?

Regards,
-- 
Jie Zhang


[-- Attachment #2: gcc-arm-fix-thumb1-pic-6.diff --]
[-- Type: text/x-diff, Size: 4112 bytes --]


	* config/arm/arm.c (thumb1_index_register_rtx_p): Remove
	declaration.
	(thumb1_index_register_rtx_p): Make global.
	(thumb1_legitimate_address_p): allow the address generated
	by calculate_pic_address pattern when !strict_p.
	* config/arm/arm-protos.h (thumb1_index_register_rtx_p): Declare.
	* config/arm/arm.md (define_split for calculate_pic_address):
	Generate valid memory address when TARGET_THUMB1.

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 168776)
+++ config/arm/arm.c	(working copy)
@@ -77,7 +77,6 @@ static int thumb2_legitimate_index_p (en
 static int thumb1_base_register_rtx_p (rtx, enum machine_mode, int);
 static rtx arm_legitimize_address (rtx, rtx, enum machine_mode);
 static rtx thumb_legitimize_address (rtx, rtx, enum machine_mode);
-inline static int thumb1_index_register_rtx_p (rtx, int);
 static bool arm_legitimate_address_p (enum machine_mode, rtx, bool);
 static int thumb_far_jump_used_p (void);
 static bool thumb_force_lr_save (void);
@@ -5889,7 +5888,7 @@ thumb1_base_register_rtx_p (rtx x, enum
 
 /* Return nonzero if x is a legitimate index register.  This is the case
    for any base register that can access a QImode object.  */
-inline static int
+int
 thumb1_index_register_rtx_p (rtx x, int strict_p)
 {
   return thumb1_base_register_rtx_p (x, QImode, strict_p);
@@ -5965,6 +5964,17 @@ thumb1_legitimate_address_p (enum machin
 	      || (!strict_p && will_be_in_index_register (XEXP (x, 1)))))
 	return 1;
 
+      /* We allow the address generated by "calculate_pic_address"
+	 pattern when !strict_p.  That pattern will be split into one
+	 of the pic_load_addr_* patterns and a move later.  */
+      else if (GET_MODE_SIZE (mode) <= 4
+	       && !strict_p
+	       && XEXP (x, 0) != frame_pointer_rtx
+	       && arm_pic_register != INVALID_REGNUM
+	       && REGNO (XEXP (x, 0)) == arm_pic_register
+	       && will_be_in_index_register (XEXP (x, 1)))
+	return 1;
+
       /* REG+const has 5-7 bit offset for non-SP registers.  */
       else if ((thumb1_index_register_rtx_p (XEXP (x, 0), strict_p)
 		|| XEXP (x, 0) == arg_pointer_rtx)
Index: config/arm/arm-protos.h
===================================================================
--- config/arm/arm-protos.h	(revision 168776)
+++ config/arm/arm-protos.h	(working copy)
@@ -49,6 +49,7 @@ extern int const_ok_for_arm (HOST_WIDE_I
 extern int arm_split_constant (RTX_CODE, enum machine_mode, rtx,
 			       HOST_WIDE_INT, rtx, rtx, int);
 extern RTX_CODE arm_canonicalize_comparison (RTX_CODE, rtx *, rtx *);
+extern int thumb1_index_register_rtx_p (rtx, int);
 extern int legitimate_pic_operand_p (rtx);
 extern rtx legitimize_pic_address (rtx, enum machine_mode, rtx);
 extern rtx legitimize_tls_address (rtx, rtx);
Index: config/arm/arm.md
===================================================================
--- config/arm/arm.md	(revision 168776)
+++ config/arm/arm.md	(working copy)
@@ -5255,10 +5255,23 @@ (define_split
 			 (unspec:SI [(match_operand:SI 2 "" "")]
 				    UNSPEC_PIC_SYM))))]
   "flag_pic"
-  [(set (match_dup 3) (unspec:SI [(match_dup 2)] UNSPEC_PIC_SYM))
-   (set (match_dup 0) (mem:SI (plus:SI (match_dup 1) (match_dup 3))))]
-  "operands[3] = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];"
-)
+  [(set (match_dup 0) (mem:SI (match_dup 3)))]
+  "
+{
+  operands[3] = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
+  emit_insn (gen_movsi (operands[3], gen_rtx_UNSPEC (SImode,
+						     gen_rtvec (1, operands[2]),
+						     UNSPEC_PIC_SYM)));
+  if (TARGET_32BIT
+      || (TARGET_THUMB1
+	  && thumb1_index_register_rtx_p (operands[1], 0)
+	  && (can_create_pseudo_p ()
+	      || thumb1_index_register_rtx_p (operands[0], 0))))
+    operands[3] = gen_rtx_PLUS (SImode, operands[1], operands[3]);
+  else
+    emit_insn (gen_movsi (operands[3],
+			  gen_rtx_PLUS (SImode, operands[1], operands[3])));
+}")
 
 ;; The rather odd constraints on the following are to force reload to leave
 ;; the insn alone, and to force the minipool generation pass to then move