[RFC,ARM] TARGET_ATOMIC_ASSIGN_EXPAND_FENV hook

Hi,

Attached patch implements TARGET_ATOMIC_ASSIGN_EXPAND_FENV for ARM. With
this, atomic test-case gcc.dg/atomic/c11-atomic-exec-5.c now PASS.

This implementation is based on SPARC and i386 implementations.

Regression tested on qemu-arm for arm-none-linux-gnueabi with no new
regression. Is this OK for trunk?

Thanks,
Kugan

gcc/
+2014-04-27  Kugan Vivekanandarajah  <kuganv@linaro.org>
+
+	* config/arm/arm.c (TARGET_ATOMIC_ASSIGN_EXPAND_FENV): New define.
+	(arm_builtins) : Add ARM_BUILTIN_LDFPSCR and ARM_BUILTIN_STFPSCR.
+	(bdesc_2arg) : Add description for builtins __builtins_arm_stfpscr
+	and __builtins_arm_ldfpscr.
+	(arm_init_builtins) : Initialize builtins __builtins_arm_stfpscr and
+	__builtins_arm_ldfpscr.
+	(arm_expand_builtin) : Expand builtins __builtins_arm_stfpscr and
+	__builtins_arm_ldfpscr.
+	(arm_atomic_assign_expand_fenv): New function.
+	* config/arm/vfp.md (stfpscr): New pattern.
+	(ldfpscr) : Likewise.
+	* config/arm/unspecs.md (unspecv): Add VUNSPEC_LDFPSCR and
+	VUNSPEC_STFPSCR.
+

On 04/26/14 11:26, Kugan wrote:
> Hi,
>
> Attached patch implements TARGET_ATOMIC_ASSIGN_EXPAND_FENV for ARM. With
> this, atomic test-case gcc.dg/atomic/c11-atomic-exec-5.c now PASS.
>
> This implementation is based on SPARC and i386 implementations.
>
> Regression tested on qemu-arm for arm-none-linux-gnueabi with no new
> regression. Is this OK for trunk?

Thanks for this patch. Can you please test this on hardware and make 
sure c11-atomic-exec-5.c works reliably ?

Testing on qemu is not enough for this patch, sorry :(.

Comments inline below.

>
> Thanks,
> Kugan
>
> gcc/
> +2014-04-27  Kugan Vivekanandarajah  <kuganv@linaro.org>
> +
> +	* config/arm/arm.c (TARGET_ATOMIC_ASSIGN_EXPAND_FENV): New define.
> +	(arm_builtins) : Add ARM_BUILTIN_LDFPSCR and ARM_BUILTIN_STFPSCR.
> +	(bdesc_2arg) : Add description for builtins __builtins_arm_stfpscr
> +	and __builtins_arm_ldfpscr.

Rename ld and st as get and set intrinsics please like AArch64.

Add __builtin_arm_setfpscr and __builtin_get_fpscr .

> +	(arm_init_builtins) : Initialize builtins __builtins_arm_stfpscr and
> +	__builtins_arm_ldfpscr.

Likewise.

> +	(arm_expand_builtin) : Expand builtins __builtins_arm_stfpscr and
> +	__builtins_arm_ldfpscr.

Likewise.

> +	(arm_atomic_assign_expand_fenv): New function.

> +	* config/arm/vfp.md (stfpscr): New pattern.
> +	(ldfpscr) : Likewise.
> +	* config/arm/unspecs.md (unspecv): Add VUNSPEC_LDFPSCR and
> +	VUNSPEC_STFPSCR.
> +
>

Replace LD and ST with Get and Set in the builtin names please overall.

> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 0240cc7..4f0ed58 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -59,6 +59,7 @@
>  #include "params.h"
>  #include "opts.h"
>  #include "dumpfile.h"
> +#include "gimple-expr.h"
>
>  /* Forward definitions of types.  */
>  typedef struct minipool_node    Mnode;
> @@ -93,6 +94,7 @@ static int thumb_far_jump_used_p (void);
>  static bool thumb_force_lr_save (void);
>  static unsigned arm_size_return_regs (void);
>  static bool arm_assemble_integer (rtx, unsigned int, int);
> +static void arm_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update);
>  static void arm_print_operand (FILE *, rtx, int);
>  static void arm_print_operand_address (FILE *, rtx);
>  static bool arm_print_operand_punct_valid_p (unsigned char code);
> @@ -584,6 +586,9 @@ static const struct attribute_spec arm_attribute_table[] =
>  #undef TARGET_MANGLE_TYPE
>  #define TARGET_MANGLE_TYPE arm_mangle_type
>
> +#undef TARGET_ATOMIC_ASSIGN_EXPAND_FENV
> +#define TARGET_ATOMIC_ASSIGN_EXPAND_FENV arm_atomic_assign_expand_fenv
> +
>  #undef TARGET_BUILD_BUILTIN_VA_LIST
>  #define TARGET_BUILD_BUILTIN_VA_LIST arm_build_builtin_va_list
>  #undef TARGET_EXPAND_BUILTIN_VA_START
> @@ -23212,6 +23217,9 @@ enum arm_builtins
>    ARM_BUILTIN_CRC32CH,
>    ARM_BUILTIN_CRC32CW,
>
> +  ARM_BUILTIN_LDFPSCR,
> +  ARM_BUILTIN_STFPSCR,
> +

s/LD/GET
s/ST/SET

>  #undef CRYPTO1
>  #undef CRYPTO2
>  #undef CRYPTO3
> @@ -24010,6 +24018,15 @@ static const struct builtin_description bdesc_2arg[] =
>    IWMMXT_BUILTIN2 (iwmmxt_wmacuz, WMACUZ)
>    IWMMXT_BUILTIN2 (iwmmxt_wmacsz, WMACSZ)
>
> +
> +#define FP_BUILTIN(L, U) \
> +  {0, CODE_FOR_##L, "__builtin_arm_"#L, ARM_BUILTIN_##U, \
> +   UNKNOWN, 0},
> +
> +  FP_BUILTIN (stfpscr, LDFPSCR)
> +  FP_BUILTIN (ldfpscr, STFPSCR)
> +#undef FP_BUILTIN
> +
>  #define CRC32_BUILTIN(L, U) \
>    {0, CODE_FOR_##L, "__builtin_arm_"#L, ARM_BUILTIN_##U, \
>     UNKNOWN, 0},
> @@ -24524,6 +24541,21 @@ arm_init_builtins (void)
>
>    if (TARGET_CRC32)
>      arm_init_crc32_builtins ();
> +
> +  if (TARGET_VFP)
> +    {
> +      tree ftype_stfpscr
> +	= build_function_type_list (void_type_node, unsigned_type_node, NULL);
> +      tree ftype_ldfpscr
> +	= build_function_type_list (unsigned_type_node, NULL);
> +
> +      arm_builtin_decls[ARM_BUILTIN_LDFPSCR]
> +	= add_builtin_function ("__builtin_arm_ldfscr", ftype_ldfpscr,
> +				ARM_BUILTIN_LDFPSCR, BUILT_IN_MD, NULL, NULL_TREE);
> +      arm_builtin_decls[ARM_BUILTIN_STFPSCR]
> +	= add_builtin_function ("__builtin_arm_stfscr", ftype_stfpscr,
> +				ARM_BUILTIN_STFPSCR, BUILT_IN_MD, NULL, NULL_TREE);
> +    }
>  }
>
>  /* Return the ARM builtin for CODE.  */
> @@ -25251,6 +25283,25 @@ arm_expand_builtin (tree exp,
>
>    switch (fcode)
>      {
> +    case ARM_BUILTIN_LDFPSCR:
> +    case ARM_BUILTIN_STFPSCR:
> +      if (fcode == ARM_BUILTIN_LDFPSCR)
> +	{
> +	  icode = CODE_FOR_ldfpscr;
> +	  target = gen_reg_rtx (SImode);
> +	  pat = GEN_FCN (icode) (target);
> +	}
> +      else
> +	{
> +	  target = NULL_RTX;
> +	  icode = CODE_FOR_stfpscr;
> +	  arg0 = CALL_EXPR_ARG (exp, 0);
> +	  op0 = expand_normal (arg0);
> +	  pat = GEN_FCN (icode) (op0);
> +	}
> +      emit_insn (pat);
> +      return target;
> +
>      case ARM_BUILTIN_TEXTRMSB:
>      case ARM_BUILTIN_TEXTRMUB:
>      case ARM_BUILTIN_TEXTRMSH:
> @@ -31116,4 +31167,70 @@ arm_asan_shadow_offset (void)
>    return (unsigned HOST_WIDE_INT) 1 << 29;
>  }
>
> +static void
> +arm_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
> +{
> +  if (!TARGET_VFP)
> +    return;
> +
> +  const unsigned FE_INVALID = 1;
> +  const unsigned FE_DIVBYZERO = 2;
> +  const unsigned FE_OVERFLOW = 4;
> +  const unsigned FE_UNDERFLOW = 8;
> +  const unsigned FE_INEXACT = 16;
> +  const unsigned HOST_WIDE_INT FE_ALL_EXCEPT = (FE_INVALID | FE_DIVBYZERO
> +						| FE_OVERFLOW | FE_UNDERFLOW
> +						| FE_INEXACT);
> +  const unsigned HOST_WIDE_INT FE_EXCEPT_SHIFT = 8;
> +
> +  /* Genareate the equivalence of :

s/Genareate/Generate.

> +       unsigned int fenv_var;
> +       fenv_var = __builtin_arm_ldfpscr ();
> +
> +       unsigned int masked_fenv;
> +       tmp1_var = fenv_var & ~ mask;
> +
> +       __builtin_arm_fpscr (&tmp1_var);  */

__builtin_arm_fpscr (&tmp1_var)? I'm not sure I follow . Fix comment here.

> +
> +  tree fenv_var = create_tmp_var (unsigned_type_node, NULL);
> +  tree ldfpscr = arm_builtin_decls[ARM_BUILTIN_LDFPSCR];
> +  tree stfpscr = arm_builtin_decls[ARM_BUILTIN_STFPSCR];
> +  tree mask = build_int_cst (unsigned_type_node,
> +			     ~((FE_ALL_EXCEPT << FE_EXCEPT_SHIFT)
> +			       | FE_ALL_EXCEPT));
> +  tree ld_fenv_stmt = build2 (MODIFY_EXPR, unsigned_type_node,
> +			      fenv_var, build_call_expr (ldfpscr, 0));
> +  tree masked_fenv = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_var, mask);
> +  tree hold_fnclex = build_call_expr (stfpscr, 1, masked_fenv);
> +  *hold = build2 (COMPOUND_EXPR, void_type_node,
> +		  build2 (COMPOUND_EXPR, void_type_node, masked_fenv,
> +			  ld_fenv_stmt), hold_fnclex);
> +
> +  /* Store the value of masked_fenv to clear the exceptions:
> +     __builtin_arm_stfpscr (masked_fenv);  */
> +
> +  *clear = build_call_expr (stfpscr, 1, masked_fenv);
> +
> +  /* Generate the equivalent of :
> +       unsigned int tmp2_var;

What is tmp2_var here ?

> +       tmp_var = __builtin_arm_fpscr ();


__builtin_arm_ldfpscr () ?

replace by __builtin_arm_getfpscr () ?


> +
> +       __builtin_arm_stfpscr (fenv_var);
> +
> +       __atomic_feraiseexcept (tmp_var);  */

Comments please in sync with code ?

> +
> +  tree tmp_var = create_tmp_var (unsigned_type_node, NULL);
> +  tree reload_fenv_stmt = build2 (MODIFY_EXPR, unsigned_type_node,
> +				  tmp_var, build_call_expr (ldfpscr, 0));
> +  tree restore_fnenv = build_call_expr (stfpscr, 1, fenv_var);
> +  tree atomic_feraiseexcept
> +    = builtin_decl_implicit (BUILT_IN_ATOMIC_FERAISEEXCEPT);
> +  tree update_call
> +    = build_call_expr (atomic_feraiseexcept, 1,
> +		       fold_convert (integer_type_node, tmp_var));
> +  *update = build2 (COMPOUND_EXPR, void_type_node,
> +		    build2 (COMPOUND_EXPR, void_type_node,
> +			    reload_fenv_stmt, restore_fnenv), update_call);
> +}
> +
>  #include "gt-arm.h"
> diff --git a/gcc/config/arm/unspecs.md b/gcc/config/arm/unspecs.md
> index 8caa953..32652c6 100644
> --- a/gcc/config/arm/unspecs.md
> +++ b/gcc/config/arm/unspecs.md
> @@ -143,6 +143,8 @@
>    VUNSPEC_SLX		; Represent a store-register-release-exclusive.
>    VUNSPEC_LDA		; Represent a store-register-acquire.
>    VUNSPEC_STL		; Represent a store-register-release.
> +  VUNSPEC_LDFPSCR	; load floating point status and control register.
> +  VUNSPEC_STFPSCR	; store floating point status and control register.
>  ])
>
>  ;; Enumerators for NEON unspecs.
> diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
> index e1a48ee..a9e4b5b 100644
> --- a/gcc/config/arm/vfp.md
> +++ b/gcc/config/arm/vfp.md
> @@ -1322,6 +1322,22 @@
>     (set_attr "conds" "unconditional")]
>  )
>
> +;; Write Floating-point Status Register.
> +(define_insn "stfpscr"
> +  [(unspec_volatile [(match_operand:SI 0 "register_operand" "r")] VUNSPEC_STFPSCR)]
> +  "TARGET_VFP"
> +  "mcr\\tp10, 7, %0, cr1, cr0, 0\\t @STFPSCR"
> +  [(set_attr "type" "mrs")])
> +
> +;; Read Floating-point Status Register.
> +(define_insn "ldfpscr"
> +  [(set (match_operand:SI 0 "register_operand" "=r")
> +        (unspec_volatile:SI [(const_int 0)] VUNSPEC_LDFPSCR))]
> +  "TARGET_VFP"
> +  "mrc\\tp10, 7, %0, cr1, cr0, 0\\t @LDFPSCR"
> +  [(set_attr "type" "mrs")])
> +
> +
>  ;; Unimplemented insns:
>  ;; fldm*
>  ;; fstm*
>

Please respin.

Ramana

It doesn't seem a good idea to me for a host-side GCC file to use the FE_* 
names for the target's FE_* values; you'd run into problems if that file 
ever ends up including the host's <fenv.h>, directly or indirectly, on any 
host.  The same comment applies to the AArch64 patch as well.

Instead I suggest names such as ARM_FE_* that won't conflict with the 
host's system headers.