[PATCH 0/4][CFT] Handle legacy __sync libcalls

[PATCH 0/4][CFT] Handle legacy __sync libcalls

[Richard Henderson]

These are the targets that used external __sync calls in gcc 4.6.
I've been intending to test them myself, but since these aren't
bare *-elf targets, it's taking me some time to get the various
cross-environment set up.

Port maintainers, please test.


r~


Richard Henderson (4):
  arm: Install __sync libfuncs for Linux.
  mips: Install the __sync libfuncs for mips16
  hppa: Install __sync libfuncs for linux.
  sh-linux: Install __sync libfuncs.

 gcc/config/arm/arm.c     |    4 ++++
 gcc/config/mips/mips.c   |    8 ++++++--
 gcc/config/pa/pa-linux.h |    3 +++
 gcc/config/pa/pa.c       |    3 +++
 gcc/config/pa/pa.h       |    5 +++++
 gcc/config/sh/linux.h    |    4 ++++
 gcc/config/sh/sh.c       |    8 ++++++++
 7 files changed, 33 insertions(+), 2 deletions(-)

-- 
1.7.6.4

[PATCH 2/4] mips: Install the __sync libfuncs for mips16

[Richard Henderson]

Cc: Richard Sandiford <rdsandiford@googlemail.com>
---
 gcc/config/mips/mips.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index ff72e28..75e73bd 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -11218,9 +11218,13 @@ mips_init_libfuncs (void)
     }
 
   /* The MIPS16 ISA does not have an encoding for "sync", so we rely
-     on an external non-MIPS16 routine to implement __sync_synchronize.  */
+     on an external non-MIPS16 routine to implement __sync_synchronize.
+     Similarly for the rest of the ll/sc libfuncs.  */
   if (TARGET_MIPS16)
-    synchronize_libfunc = init_one_libfunc ("__sync_synchronize");
+    {
+      synchronize_libfunc = init_one_libfunc ("__sync_synchronize");
+      init_sync_libfuncs (UNITS_PER_WORD);
+    }
 }
 
 /* Build up a multi-insn sequence that loads label TARGET into $AT.  */
-- 
1.7.6.4

Re: [PATCH 2/4] mips: Install the __sync libfuncs for mips16

[Richard Sandiford]

Richard Henderson <rth@redhat.com> writes:
>  gcc/config/mips/mips.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index ff72e28..75e73bd 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -11218,9 +11218,13 @@ mips_init_libfuncs (void)
>      }
>  
>    /* The MIPS16 ISA does not have an encoding for "sync", so we rely
> -     on an external non-MIPS16 routine to implement __sync_synchronize.  */
> +     on an external non-MIPS16 routine to implement __sync_synchronize.
> +     Similarly for the rest of the ll/sc libfuncs.  */
>    if (TARGET_MIPS16)
> -    synchronize_libfunc = init_one_libfunc ("__sync_synchronize");
> +    {
> +      synchronize_libfunc = init_one_libfunc ("__sync_synchronize");
> +      init_sync_libfuncs (UNITS_PER_WORD);
> +    }
>  }
>  
>  /* Build up a multi-insn sequence that loads label TARGET into $AT.  */

Finally tested on mips64-linux-gnu with -mabi=32/-mips16 multilibs
(sorry for the delay).  Results look good, so please go ahead.

Thanks,
Richard

[PATCH 3/4] hppa: Install __sync libfuncs for linux.

[Richard Henderson]

Cc: John David Anglin <dave.anglin@nrc-cnrc.gc.ca>
---
 gcc/config/pa/pa-linux.h |    3 +++
 gcc/config/pa/pa.c       |    3 +++
 gcc/config/pa/pa.h       |    5 +++++
 3 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/gcc/config/pa/pa-linux.h b/gcc/config/pa/pa-linux.h
index 6c6cf21..addc0e1 100644
--- a/gcc/config/pa/pa-linux.h
+++ b/gcc/config/pa/pa-linux.h
@@ -136,3 +136,6 @@ along with GCC; see the file COPYING3.  If not see
 /* Linux always uses gas.  */
 #undef TARGET_GAS
 #define TARGET_GAS 1
+
+#undef TARGET_SYNC_LIBCALL
+#define TARGET_SYNC_LIBCALL 1
diff --git a/gcc/config/pa/pa.c b/gcc/config/pa/pa.c
index 66574ba..134f1f8 100644
--- a/gcc/config/pa/pa.c
+++ b/gcc/config/pa/pa.c
@@ -5587,6 +5587,9 @@ pa_init_libfuncs (void)
       set_conv_libfunc (ufloat_optab, TFmode, DImode,
 			"_U_Qfcnvxf_udbl_to_quad");
     }
+
+  if (TARGET_SYNC_LIBCALL)
+    init_sync_libfuncs (UNITS_PER_WORD);
 }
 
 /* HP's millicode routines mean something special to the assembler.
diff --git a/gcc/config/pa/pa.h b/gcc/config/pa/pa.h
index 2f1295b..c52e3d5 100644
--- a/gcc/config/pa/pa.h
+++ b/gcc/config/pa/pa.h
@@ -74,6 +74,11 @@ extern unsigned long total_code_bytes;
 #define HPUX_LONG_DOUBLE_LIBRARY 0
 #endif
 
+/* Linux kernel atomic operation support.  */
+#ifndef TARGET_SYNC_LIBCALL
+#define TARGET_SYNC_LIBCALL 0
+#endif
+
 /* The following three defines are potential target switches.  The current
    defines are optimal given the current capabilities of GAS and GNU ld.  */
 
-- 
1.7.6.4

Re: [PATCH 3/4] hppa: Install __sync libfuncs for linux.

[Gerald Pfeifer]

On Fri, 11 Nov 2011, Richard Henderson wrote:
> @@ -136,3 +136,6 @@ along with GCC; see the file COPYING3.  If not see
>  /* Linux always uses gas.  */
>  #undef TARGET_GAS
>  #define TARGET_GAS 1
> +
> +#undef TARGET_SYNC_LIBCALL
> +#define TARGET_SYNC_LIBCALL 1

John, Richard, while you are at it, mind making this GNU/Linux per
guidance from RMS?  (That'll save us work later on.)

I assume the ChangeLog entry also should refer to GNU/Linux?

Gerald

Re: [PATCH 3/4] hppa: Install __sync libfuncs for linux.

[Dave Anglin]

On 12-Nov-11, at 3:42 PM, Gerald Pfeifer wrote:

> John, Richard, while you are at it, mind making this GNU/Linux per
> guidance from RMS?  (That'll save us work later on.)

Yes.  I don't want to participate in this controversy.

I believe the file was initially contributed by developers working under
a HP contract to port the PA-RISC architecture to the "Linux" kernel.

John David Anglin

Re: [PATCH 3/4] hppa: Install __sync libfuncs for linux.

[Gerald Pfeifer]

On Sat, 12 Nov 2011, Dave Anglin wrote:
>> John, Richard, while you are at it, mind making this GNU/Linux per
>> guidance from RMS?  (That'll save us work later on.)
> Yes.  I don't want to participate in this controversy.

That was more a rhetorical question. :-\

Addressing this is not really optional, so I bit the bullet and
applied the patch below myself.

Gerald


2011-11-27  Gerald Pfeifer  <gerald@pfeifer.com>

	* config/pa/pa-linux.h (TARGET_GAS): Remove comment.

Index: config/pa/pa-linux.h
===================================================================
--- config/pa/pa-linux.h	(revision 181742)
+++ config/pa/pa-linux.h	(working copy)
@@ -133,7 +133,6 @@
     }								\
   while (0)
 
-/* Linux always uses gas.  */
 #undef TARGET_GAS
 #define TARGET_GAS 1
 

Re: [PATCH 3/4] hppa: Install __sync libfuncs for linux.

[Richard Henderson]

On 11/12/2011 10:42 AM, Gerald Pfeifer wrote:
> I assume the ChangeLog entry also should refer to GNU/Linux?

Meh.  It really does refer specifically to the Linux kernel,
and not the GNU userland.


r~

Re: [PATCH 3/4] hppa: Install __sync libfuncs for linux.

[Gerald Pfeifer]

On Sun, 13 Nov 2011, Richard Henderson wrote:
>> I assume the ChangeLog entry also should refer to GNU/Linux?
> Meh.  It really does refer specifically to the Linux kernel,
> and not the GNU userland.

Ah, cool then.

The comment, however, reads
  /* Linux always uses gas.  */
which does seem to warrant GNU/Linux in RMS diction.

Gerald

Re: [PATCH 3/4] hppa: Install __sync libfuncs for linux.

[Richard Henderson]

On 11/13/2011 11:45 AM, Gerald Pfeifer wrote:
> The comment, however, reads
>   /* Linux always uses gas.  */
> which does seem to warrant GNU/Linux in RMS diction.

Oh, unrelated to my actual patch then.


r~

[PATCH 4/4] sh-linux: Install __sync libfuncs.

[Richard Henderson]

Cc: Kaz Kojima <kkojima@gcc.gnu.org>
---
 gcc/config/sh/linux.h |    4 ++++
 gcc/config/sh/sh.c    |    8 ++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/gcc/config/sh/linux.h b/gcc/config/sh/linux.h
index edfd99b..7a75341 100644
--- a/gcc/config/sh/linux.h
+++ b/gcc/config/sh/linux.h
@@ -131,3 +131,7 @@ along with GCC; see the file COPYING3.  If not see
 #define SH_DIV_STRATEGY_DEFAULT SH_DIV_CALL2
 #undef SH_DIV_STR_FOR_SIZE
 #define SH_DIV_STR_FOR_SIZE "call2"
+
+/* Install the __sync libcalls.  */
+#undef TARGET_INIT_LIBFUNCS
+#define TARGET_INIT_LIBFUNCS  sh_init_sync_libfuncs
diff --git a/gcc/config/sh/sh.c b/gcc/config/sh/sh.c
index 03c3c48..2545a63 100644
--- a/gcc/config/sh/sh.c
+++ b/gcc/config/sh/sh.c
@@ -302,6 +302,8 @@ static void sh_trampoline_init (rtx, tree, rtx);
 static rtx sh_trampoline_adjust_address (rtx);
 static void sh_conditional_register_usage (void);
 static bool sh_legitimate_constant_p (enum machine_mode, rtx);
+
+static void sh_init_sync_libfuncs (void) ATTRIBUTE_UNUSED;
 \f
 static const struct attribute_spec sh_attribute_table[] =
 {
@@ -12499,4 +12501,10 @@ sh_legitimate_constant_p (enum machine_mode mode, rtx x)
 
 enum sh_divide_strategy_e sh_div_strategy = SH_DIV_STRATEGY_DEFAULT;
 
+static void
+sh_init_sync_libfuncs (void)
+{
+  init_sync_libfuncs (UNITS_PER_WORD);
+}
+
 #include "gt-sh.h"
-- 
1.7.6.4

[PATCH 1/4] arm: Install __sync libfuncs for Linux.

[Richard Henderson]

Cc: Richard Earnshaw <richard.earnshaw@arm.com>
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
---
 gcc/config/arm/arm.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 6ef6f62..abf8ce1 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -1096,6 +1096,10 @@ arm_set_fixed_conv_libfunc (convert_optab optable, enum machine_mode to,
 static void
 arm_init_libfuncs (void)
 {
+  /* For Linux, we have access to kernel support for atomic operations.  */
+  if (arm_abi == ARM_ABI_AAPCS_LINUX)
+    init_sync_libfuncs (8);
+
   /* There are no special library functions unless we are using the
      ARM BPABI.  */
   if (!TARGET_BPABI)
-- 
1.7.6.4

Re: [PATCH 1/4] arm: Install __sync libfuncs for Linux.

[David Gilbert]

On 11 November 2011 23:32, Richard Henderson <rth@redhat.com> wrote:
> Cc: Richard Earnshaw <richard.earnshaw@arm.com>
> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> ---
>  gcc/config/arm/arm.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 6ef6f62..abf8ce1 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -1096,6 +1096,10 @@ arm_set_fixed_conv_libfunc (convert_optab optable, enum machine_mode to,
>  static void
>  arm_init_libfuncs (void)
>  {
> +  /* For Linux, we have access to kernel support for atomic operations.  */
> +  if (arm_abi == ARM_ABI_AAPCS_LINUX)
> +    init_sync_libfuncs (8);

There is unfortunately no guarantee that your kernel has support for the 64bit
cases, since this was only recently added - and the libgcc code checks
and aborts if linked in.

(As far as I can tell there is approximately one potential user of 64bit atomics
on ARM, so it's important not to do that check for all binaries).

Dave

Re: [PATCH 1/4] arm: Install __sync libfuncs for Linux.

[Richard Henderson]

On 11/14/2011 08:08 AM, David Gilbert wrote:
> There is unfortunately no guarantee that your kernel has support for the 64bit
> cases, since this was only recently added - and the libgcc code checks
> and aborts if linked in.
> 
> (As far as I can tell there is approximately one potential user of 64bit atomics
> on ARM, so it's important not to do that check for all binaries).

Yes, I know.  But that's something the user had to deal with before my patch too.
But if I just put UNITS_PER_WORD there, you'll won't be able to use those routines
even if the kernel support is present.

This is something that is more ideally handled by the atomics library that is
being discussed; __atomic_is_lock_free would be able to query the existence of
the kernel support in reporting its result.

Andrew, what's the state of discussion on the atomics library?  While I 
understand that vendors want to be able to replace that library, and thus we
want to settle on a standard ABI, I think *not* providing at least a reference
copy with GCC is almost certainly a mistake.


r~

Re: [PATCH 1/4] arm: Install __sync libfuncs for Linux.

[Andrew MacLeod]

On 11/14/2011 01:18 PM, Richard Henderson wrote:
> On 11/14/2011 08:08 AM, David Gilbert wrote:
>
> Andrew, what's the state of discussion on the atomics library?  While I
> understand that vendors want to be able to replace that library, and thus we
> want to settle on a standard ABI, I think *not* providing at least a reference
> copy with GCC is almost certainly a mistake.

There should be one sometime in the next few weeks...  I have no 
intention of providing a compiler without SOMETHING that people can 
acquire and build.  It just may not be part of the compiler, but will be 
easily available.

Andrew

Re: [PATCH 1/4] arm: Install __sync libfuncs for Linux.

[Ramana Radhakrishnan]

>
> There is unfortunately no guarantee that your kernel has support for the 64bit
> cases, since this was only recently added - and the libgcc code checks
> and aborts if linked in.

I don't see how this patch will change status-quo.

Richard: I've not been able to test the patch yet given that trunk is
broken for arm-linux-gnueabi currently. I will probably have to try
doing this sometime later tonight or so.

cheers
Ramana

Re: [PATCH 0/4][CFT] Handle legacy __sync libcalls

[Kaz Kojima]

Richard Henderson <rth@redhat.com> wrote:
> These are the targets that used external __sync calls in gcc 4.6.
> I've been intending to test them myself, but since these aren't
> bare *-elf targets, it's taking me some time to get the various
> cross-environment set up.
> 
> Port maintainers, please test.

SH patch looks to work fine, though I've got an ICE when
regtesting:

FAIL: gcc.c-torture/compile/20061005-1.c  -O0  (internal compiler error)

In function 'testc2':
trunk/gcc/testsuite/gcc.c-torture/compile/20061005-1.c:22:3: internal compiler error: in emit_move_insn, at expr.c:3438

#0  fancy_abort (file=0x89448fc "../../LOCAL/trunk/gcc/expr.c", line=3438, 
    function=0x894553b "emit_move_insn")
    at ../../LOCAL/trunk/gcc/diagnostic.c:899
#1  0x082a7e92 in emit_move_insn (x=0xb7de633c, y=0xb7e71970)
    at ../../LOCAL/trunk/gcc/expr.c:3437
#2  0x081b8000 in emit_library_call_value_1 (retval=1, orgfun=0xb7e2c770, 
    value=0xb7de633c, fn_type=LCT_NORMAL, outmode=QImode, nargs=3, 
    p=<value optimized out>) at ../../LOCAL/trunk/gcc/calls.c:4103
#3  0x081b8271 in emit_library_call_value (orgfun=0xb7e2c770, 
    value=0xb7de633c, fn_type=LCT_NORMAL, outmode=QImode, nargs=3)
    at ../../LOCAL/trunk/gcc/calls.c:4184
#4  0x08444ea7 in expand_atomic_compare_and_swap (ptarget_bool=0x0, 
    ptarget_oval=0xbfffeba0, mem=0xb7e42654, expected=0xb7de6330, 
    desired=0xb7de6318, is_weak=0 '\000', succ_model=MEMMODEL_SEQ_CST, 
    fail_model=MEMMODEL_SEQ_CST) at ../../LOCAL/trunk/gcc/optabs.c:7513
#5  0x0818b26f in expand_builtin_compare_and_swap (mode=QImode, 
    exp=<value optimized out>, is_bool=0 '\000', target=0xb7de633c)
    at ../../LOCAL/trunk/gcc/builtins.c:5199

(gdb) fr 1
#1  0x082a7e92 in emit_move_insn (x=0xb7de633c, y=0xb7e71970)
    at ../../LOCAL/trunk/gcc/expr.c:3437
3437	  gcc_assert (mode != BLKmode

(gdb) call debug_rtx(x)
(const_int 0 [0])

(gdb) fr 6
#6  0x0819d19b in expand_builtin (exp=0xb7e622ec, target=0xb7de633c, 
    subtarget=0x0, mode=<value optimized out>, ignore=1)
    at ../../LOCAL/trunk/gcc/builtins.c:6529
6529	      target = expand_builtin_compare_and_swap (mode, exp, false, target);

It seems that expand_builtin sets "target" variable to
const0_trx when "ignore" argument is set and this causes
the above ICE.  I'm trying a patch

--- ORIG/trunk/gcc/optabs.c	2011-11-11 08:00:04.000000000 +0900
+++ trunk/gcc/optabs.c	2011-11-12 12:34:18.000000000 +0900
@@ -7440,6 +7440,7 @@ expand_atomic_compare_and_swap (rtx *pta
      just in case we need that path down below.  */
   if (ptarget_oval == NULL
       || (target_oval = *ptarget_oval) == NULL
+      || !register_operand (target_oval, mode)
       || reg_overlap_mentioned_p (expected, target_oval))
     target_oval = gen_reg_rtx (mode);
 
though I'm not sure that this is the right thing to do.

Regards,
	kaz

Re: [PATCH 0/4][CFT] Handle legacy __sync libcalls

[Richard Henderson]

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

On 11/11/2011 07:39 PM, Kaz Kojima wrote:
> It seems that expand_builtin sets "target" variable to
> const0_trx when "ignore" argument is set and this causes
> the above ICE.  I'm trying a patch ...

I think the fix belongs in expand_builtin_compare_and_swap.
I'm testing the following.


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 2059 bytes --]

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 98dc636..9dc68cc 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -5144,7 +5144,6 @@ expand_builtin_sync_operation (enum machine_mode mode, tree exp,
 	case BUILT_IN_SYNC_FETCH_AND_NAND_4:
 	case BUILT_IN_SYNC_FETCH_AND_NAND_8:
 	case BUILT_IN_SYNC_FETCH_AND_NAND_16:
-
 	  if (warned_f_a_n)
 	    break;
 
@@ -5158,7 +5157,6 @@ expand_builtin_sync_operation (enum machine_mode mode, tree exp,
 	case BUILT_IN_SYNC_NAND_AND_FETCH_4:
 	case BUILT_IN_SYNC_NAND_AND_FETCH_8:
 	case BUILT_IN_SYNC_NAND_AND_FETCH_16:
-
 	  if (warned_n_a_f)
 	    break;
 
@@ -5190,16 +5188,24 @@ expand_builtin_compare_and_swap (enum machine_mode mode, tree exp,
 				 bool is_bool, rtx target)
 {
   rtx old_val, new_val, mem;
+  rtx *pbool, *poval;
 
   /* Expand the operands.  */
   mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
   old_val = expand_expr_force_mode (CALL_EXPR_ARG (exp, 1), mode);
   new_val = expand_expr_force_mode (CALL_EXPR_ARG (exp, 2), mode);
 
-  if (!expand_atomic_compare_and_swap ((is_bool ? &target : NULL),
-				       (is_bool ? NULL : &target),
-				       mem, old_val, new_val, false,
-				       MEMMODEL_SEQ_CST, MEMMODEL_SEQ_CST))
+  pbool = poval = NULL;
+  if (target != const0_rtx)
+    {
+      if (is_bool)
+	pbool = ⌖
+      else
+	poval = ⌖
+    }
+  if (!expand_atomic_compare_and_swap (pbool, poval, mem, old_val, new_val,
+				       false, MEMMODEL_SEQ_CST,
+				       MEMMODEL_SEQ_CST))
     return NULL_RTX;
 
   return target;
@@ -5338,8 +5344,9 @@ expand_builtin_atomic_compare_exchange (enum machine_mode mode, tree exp,
 
   oldval = copy_to_reg (gen_rtx_MEM (mode, expect));
 
-  if (!expand_atomic_compare_and_swap (&target, &oldval, mem, oldval,
-				       desired, is_weak, success, failure))
+  if (!expand_atomic_compare_and_swap ((target == const0_rtx ? NULL : &target),
+				       &oldval, mem, oldval, desired,
+				       is_weak, success, failure))
     return NULL_RTX;
 
   emit_move_insn (gen_rtx_MEM (mode, expect), oldval);

Re: [PATCH 0/4][CFT] Handle legacy __sync libcalls

[Richard Henderson]

On 11/12/2011 07:56 AM, Richard Henderson wrote:
> On 11/11/2011 07:39 PM, Kaz Kojima wrote:
>> It seems that expand_builtin sets "target" variable to
>> const0_trx when "ignore" argument is set and this causes
>> the above ICE.  I'm trying a patch ...
> 
> I think the fix belongs in expand_builtin_compare_and_swap.
> I'm testing the following.

Full test completed on x86_64-linux.  I verified that the test
you mentioned no longer ICEs on sh4-linux.

Committed.


r~

Re: [PATCH 0/4][CFT] Handle legacy __sync libcalls

[Kaz Kojima]

Richard Henderson <rth@redhat.com> wrote:
>> I think the fix belongs in expand_builtin_compare_and_swap.
>> I'm testing the following.
> 
> Full test completed on x86_64-linux.  I verified that the test
> you mentioned no longer ICEs on sh4-linux.
> 
> Committed.

Thanks!

Regards,
	kaz