Re: lower-subreg and IBM long double

Re: lower-subreg and IBM long double

[Alan Modra]

On Tue, Jun 11, 2013 at 04:58:07PM +0000, Joseph S. Myers wrote:
> It's preferred to put the hook documentation in the doc string in 
> target.def, so tm.texi.in then only contains the @hook line indicating 
> where the documentation will go in tm.texi.

Ah, thanks.  Revised patch with testcase.

gcc/
	* rs6000.c (TARGET_INIT_LOWER_SUBREG): Define.
	(rs6000_init_lower_subreg): New function.
	* lower-subreg.c (init_lower_subreg): Call targetm.init_lower_subreg.
	* target.def (init_lower_subreg): New.
	* doc/tm.texi.in (TARGET_INIT_LOWER_SUBREG): Document.
	* doc/tm.texi: Regenerate.
gcc/testsuite/
	* gcc.target/powerpc/fabsl.c: New test.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 199975)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -59,6 +59,7 @@
 #include "opts.h"
 #include "tree-vectorizer.h"
 #include "dumpfile.h"
+#include "lower-subreg.h"
 #if TARGET_XCOFF
 #include "xcoffout.h"  /* get declarations of xcoff_*_section_name */
 #endif
@@ -1357,6 +1358,8 @@ static const struct attribute_spec rs6000_attribut
 #define TARGET_RTX_COSTS rs6000_rtx_costs
 #undef TARGET_ADDRESS_COST
 #define TARGET_ADDRESS_COST hook_int_rtx_mode_as_bool_0
+#undef TARGET_INIT_LOWER_SUBREG
+#define TARGET_INIT_LOWER_SUBREG rs6000_init_lower_subreg
 
 #undef TARGET_DWARF_REGISTER_SPAN
 #define TARGET_DWARF_REGISTER_SPAN rs6000_dwarf_register_span
@@ -27430,6 +27523,20 @@ rs6000_memory_move_cost (enum machine_mode mode, r
   return ret;
 }
 
+static void
+rs6000_init_lower_subreg (void *data)
+{
+  if (!TARGET_IEEEQUAD
+      && TARGET_HARD_FLOAT
+      && (TARGET_FPRS || TARGET_E500_DOUBLE)
+      && TARGET_LONG_DOUBLE_128)
+    {
+      struct target_lower_subreg *info = (struct target_lower_subreg *) data;
+      info->x_choices[0].move_modes_to_split[TFmode] = false;
+      info->x_choices[1].move_modes_to_split[TFmode] = false;
+    }
+}
+
 /* Returns a code for a target-specific builtin that implements
    reciprocal of the function, or NULL_TREE if not available.  */
 
Index: gcc/lower-subreg.c
===================================================================
--- gcc/lower-subreg.c	(revision 199975)
+++ gcc/lower-subreg.c	(working copy)
@@ -39,6 +39,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-pass.h"
 #include "df.h"
 #include "lower-subreg.h"
+#include "target.h"
 
 #ifdef STACK_GROWS_DOWNWARD
 # undef STACK_GROWS_DOWNWARD
@@ -287,6 +288,9 @@ init_lower_subreg (void)
   if (LOG_COSTS)
     fprintf (stderr, "\nSpeed costs\n===========\n\n");
   compute_costs (true, &rtxes);
+
+  if (targetm.init_lower_subreg)
+    targetm.init_lower_subreg (this_target_lower_subreg);
 }
 
 static bool
Index: gcc/target.def
===================================================================
--- gcc/target.def	(revision 199975)
+++ gcc/target.def	(working copy)
@@ -2926,6 +2926,14 @@ DEFHOOK
  void, (int *code, rtx *op0, rtx *op1, bool op0_preserve_value),
  default_canonicalize_comparison)
 
+/* Allow modification of subreg choices.  */
+DEFHOOK
+(init_lower_subreg,
+ "This hook allows modification of the choices the lower_subreg pass \
+will make for particular subreg modes.  @var{data} is a pointer to a \
+@code{struct target_lower_subreg}.",
+ void, (void *data), NULL)
+
 DEFHOOKPOD
 (atomic_test_and_set_trueval,
  "This value should be set if the result written by\
Index: gcc/doc/tm.texi.in
===================================================================
--- gcc/doc/tm.texi.in	(revision 199975)
+++ gcc/doc/tm.texi.in	(working copy)
@@ -6384,6 +6384,8 @@ should probably only be given to addresses with di
 registers on machines with lots of registers.
 @end deftypefn
 
+@hook TARGET_INIT_LOWER_SUBREG
+
 @node Scheduling
 @section Adjusting the Instruction Scheduler
 
Index: gcc/testsuite/gcc.target/powerpc/fabsl.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/fabsl.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/fabsl.c	(revision 0)
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { powerpc*-*-darwin* powerpc*-*-aix* rs6000-*-* powerpc*-*-linux* } } } */
+/* { dg-options "-O2 -mlong-double-128" } */
+/* { dg-final { scan-assembler-not "lwz" } } */
+/* { dg-final { scan-assembler-not "ld" } } */
+
+long double
+_fabsl (long double x)
+{
+  return __builtin_fabsl (x);
+}


-- 
Alan Modra
Australia Development Lab, IBM

[PATCH] lower-subreg and IBM long double

[Alan Modra]

On Tue, Jun 11, 2013 at 09:56:11AM +0930, Alan Modra wrote:
>[snip] 
> It isn't hard to see why we are going wrong.  IBM long double is
> really a two element array of double, and the rs6000 backend uses
> subregs to access the elements.  The problem is that lower-subreg
> lowers to word_mode, so we get DImode.  word_mode makes sense for most
> targets where subregs of FP modes might be used to narrow an access
> for bit-twiddling operations on the sign bit.  It doesn't make sense
> for us.  We want DFmode for FP operations.  An example is the expander
> used by the testcase.

This is a repost of http://gcc.gnu.org/ml/gcc/2013-06/msg00053.html,
taking into account Joseph's doc review comments and adding a
testcase.  David has already acked the rs6000.c changes.  Bootstrapped
and regression tested powerpc64-linux.  OK to apply?

gcc/
	* rs6000.c (TARGET_INIT_LOWER_SUBREG): Define.
	(rs6000_init_lower_subreg): New function.
	* lower-subreg.c (init_lower_subreg): Call targetm.init_lower_subreg.
	* target.def (init_lower_subreg): New.
	* doc/tm.texi.in (TARGET_INIT_LOWER_SUBREG): Document.
	* doc/tm.texi: Regenerate.
gcc/testsuite/
	* gcc.target/powerpc/fabsl.c: New test.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 201834)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -59,6 +59,7 @@
 #include "opts.h"
 #include "tree-vectorizer.h"
 #include "dumpfile.h"
+#include "lower-subreg.h"
 #if TARGET_XCOFF
 #include "xcoffout.h"  /* get declarations of xcoff_*_section_name */
 #endif
@@ -1364,6 +1365,8 @@ static const struct attribute_spec rs6000_attribut
 #define TARGET_RTX_COSTS rs6000_rtx_costs
 #undef TARGET_ADDRESS_COST
 #define TARGET_ADDRESS_COST hook_int_rtx_mode_as_bool_0
+#undef TARGET_INIT_LOWER_SUBREG
+#define TARGET_INIT_LOWER_SUBREG rs6000_init_lower_subreg
 
 #undef TARGET_DWARF_REGISTER_SPAN
 #define TARGET_DWARF_REGISTER_SPAN rs6000_dwarf_register_span
@@ -27971,6 +27974,20 @@ rs6000_memory_move_cost (enum machine_mode mode, r
   return ret;
 }
 
+static void
+rs6000_init_lower_subreg (void *data)
+{
+  if (!TARGET_IEEEQUAD
+      && TARGET_HARD_FLOAT
+      && (TARGET_FPRS || TARGET_E500_DOUBLE)
+      && TARGET_LONG_DOUBLE_128)
+    {
+      struct target_lower_subreg *info = (struct target_lower_subreg *) data;
+      info->x_choices[0].move_modes_to_split[TFmode] = false;
+      info->x_choices[1].move_modes_to_split[TFmode] = false;
+    }
+}
+
 /* Returns a code for a target-specific builtin that implements
    reciprocal of the function, or NULL_TREE if not available.  */
 
Index: gcc/lower-subreg.c
===================================================================
--- gcc/lower-subreg.c	(revision 201834)
+++ gcc/lower-subreg.c	(working copy)
@@ -39,6 +39,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-pass.h"
 #include "df.h"
 #include "lower-subreg.h"
+#include "target.h"
 
 #ifdef STACK_GROWS_DOWNWARD
 # undef STACK_GROWS_DOWNWARD
@@ -287,6 +288,9 @@ init_lower_subreg (void)
   if (LOG_COSTS)
     fprintf (stderr, "\nSpeed costs\n===========\n\n");
   compute_costs (true, &rtxes);
+
+  if (targetm.init_lower_subreg)
+    targetm.init_lower_subreg (this_target_lower_subreg);
 }
 
 static bool
Index: gcc/target.def
===================================================================
--- gcc/target.def	(revision 201834)
+++ gcc/target.def	(working copy)
@@ -5110,6 +5110,14 @@ comparison code or operands.",
  void, (int *code, rtx *op0, rtx *op1, bool op0_preserve_value),
  default_canonicalize_comparison)
 
+/* Allow modification of subreg choices.  */
+DEFHOOK
+(init_lower_subreg,
+ "This hook allows modification of the choices the lower_subreg pass \
+will make for particular subreg modes.  @var{data} is a pointer to a \
+@code{struct target_lower_subreg}.",
+ void, (void *data), NULL)
+
 DEFHOOKPOD
 (atomic_test_and_set_trueval,
  "This value should be set if the result written by\
Index: gcc/doc/tm.texi.in
===================================================================
--- gcc/doc/tm.texi.in	(revision 201834)
+++ gcc/doc/tm.texi.in	(working copy)
@@ -4939,6 +4939,8 @@ Define this macro if a non-short-circuit operation
 
 @hook TARGET_ADDRESS_COST
 
+@hook TARGET_INIT_LOWER_SUBREG
+
 @node Scheduling
 @section Adjusting the Instruction Scheduler
 


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] lower-subreg and IBM long double

[Steven Bosscher]

On Mon, Aug 19, 2013 at 8:47 AM, Alan Modra wrote:
> gcc/
...
>         * doc/tm.texi.in (TARGET_INIT_LOWER_SUBREG): Document.

ChangeLog update needed here, the new hook is documented in target.def.


> +static void
> +rs6000_init_lower_subreg (void *data)
> +{

Functions should start with a leading comment.


> Index: gcc/target.def
> ===================================================================
> --- gcc/target.def      (revision 201834)
> +++ gcc/target.def      (working copy)
> @@ -5110,6 +5110,14 @@ comparison code or operands.",
>   void, (int *code, rtx *op0, rtx *op1, bool op0_preserve_value),
>   default_canonicalize_comparison)
>
> +/* Allow modification of subreg choices.  */
> +DEFHOOK
> +(init_lower_subreg,
> + "This hook allows modification of the choices the lower_subreg pass \
> +will make for particular subreg modes.  @var{data} is a pointer to a \
> +@code{struct target_lower_subreg}.",
> + void, (void *data), NULL)
> +

Bah, another void* pointer...

I would have preferred a hook that takes a machine_mode and let it be
called in the first loop of compute_costs, something like the code
below.

Your solution gives the target a lot more power to control the split
decisions (adjust individual costs, review overall choice of modes to
split, etc.) but I don't think any target really needs that. Do you
think the target should have more control over splitting than just on
per machine_mode basis?

Ciao!
Steven


Index: lower-subreg.c
===================================================================
--- lower-subreg.c      (revision 201887)
+++ lower-subreg.c      (working copy)
@@ -208,7 +208,18 @@ compute_costs (bool speed_p, struct cost_rtxes *rt
       if (factor > 1)
        {
          int mode_move_cost;
-
+         bool split_profitable_mode = true;
+
+         if (targetm.split_profitable_mode
+             && ! targetm.split_profitable_mode ((mode)))
+           {
+             split_profitable_mode = false;
+             if (LOG_COSTS)
+               fprintf (stderr,
+                        "%s move: target signals splitting is not
profitable%s\n",
+                        GET_MODE_NAME (mode), FORCE_LOWERING ? "
(forcing)" : "");
+           }
+
          PUT_MODE (rtxes->target, mode);
          PUT_MODE (rtxes->source, mode);
          mode_move_cost = set_rtx_cost (rtxes->set, speed_p);
@@ -218,7 +229,9 @@ compute_costs (bool speed_p, struct cost_rtxes *rt
                     GET_MODE_NAME (mode), mode_move_cost,
                     word_move_cost, factor);

-         if (FORCE_LOWERING || mode_move_cost >= word_move_cost * factor)
+         if (FORCE_LOWERING
+             || ((mode_move_cost >= word_move_cost * factor)
+                 && split_profitable_mode))
            {
              choices[speed_p].move_modes_to_split[i] = true;
              choices[speed_p].something_to_do = true;