Patchwork [SMS] Prevent the creation of reg-moves for definitions with MODE_CC

login
register
mail settings
Submitter Revital Eres
Date Dec. 20, 2011, 2:13 p.m.
Message ID <CAHz1=dWh6nP9kDj8SnSRqz970z+ikV=5f3Yb5syL6h00im_FrA@mail.gmail.com>
Download mbox | patch
Permalink /patch/132432/
State New
Headers show

Comments

Revital Eres - Dec. 20, 2011, 2:13 p.m.
Hello,

The testcase attached causes ICE when compiling with
-fmodulo-sched-allow-regmoves on ARM due to reg-moves created for the
definition of mode MODE_CC.

The following is a snippet from the ddg of the definition and use of vfpcc
which triggers the creation of the reg-move:

Node num: 1
(insn 151 77 152 6 (set (reg:CCFP 127 vfpcc)
        (compare:CCFP (reg:SF 202 [ MEM[base: D.5306_32, offset: 0B] ])
            (reg:SF 183 [ D.5284 ]))) test_new.c:8 694 {*cmpsf_vfp}
     (expr_list:REG_DEAD (reg:SF 202 [ MEM[base: D.5306_32, offset: 0B] ])
        (nil)))
OUT ARCS:  [151 -(T,4,0)-> 152]
IN ARCS:  [77 -(T,3,0)-> 151]
Node num: 2
(insn 152 151 120 6 (set (reg:CCFP 24 cc)
        (reg:CCFP 127 vfpcc)) test_new.c:8 689 {*movcc_vfp}
     (expr_list:REG_DEAD (reg:CCFP 127 vfpcc)
        (nil)))
OUT ARCS:  [152 -(O,0,0)-> 144]  [152 -(T,0,0)-> 120]
IN ARCS:  [145 -(A,0,1)-> 152]  [151 -(T,4,0)-> 152]

The attached patch prevents the creation of reg-moves for definitions
with MODE_CC and thus solves this ICE.

Currently testing and bootstrap on ppc64-redhat-linux, enabling SMS on
loops with SC 1.

OK for 4.7 once testing completes?

Thanks,
Revital

Changelog:

gcc/
        * ddg.c (def_has_ccmode_p): New function.
        (add_cross_iteration_register_deps): Call it.

testsuite/
         * gcc.dg/sms-11.c: New file.
Richard Sandiford - Dec. 20, 2011, 5:47 p.m.
Revital Eres <revital.eres@linaro.org> writes:
> +/* Return true if one of the definitions in INSN has MODE_CC.  Otherwise
> +   return false.  */
> +static bool
> +def_has_ccmode_p (rtx insn)
> +{
> +  df_ref *def;
> +
> +  for (def = DF_INSN_DEFS (insn); *def; def++)
> +    {
> +      enum machine_mode mode = GET_MODE (DF_REF_REG (*def));
> +
> +      if (GET_MODE_CLASS (mode) == MODE_CC)
> +       return true;
> +    }
> +
> +  return false;
> +}

FWIW, an alternative might be to test have_regs_of_mode[(int) mode].
That says whether there are any allocatable (non-fixed) registers
of the given mode.

Richard
Revital Eres - Dec. 20, 2011, 8:19 p.m.
Hello,

> FWIW, an alternative might be to test have_regs_of_mode[(int) mode].
> That says whether there are any allocatable (non-fixed) registers
> of the given mode.

Thanks, I'll prepare a new version of the patch using have_regs_of_mode.

Revital

>
> Richard
Richard Henderson - Dec. 21, 2011, 7:46 p.m.
On 12/20/2011 09:47 AM, Richard Sandiford wrote:
> Revital Eres <revital.eres@linaro.org> writes:
>> +/* Return true if one of the definitions in INSN has MODE_CC.  Otherwise
>> +   return false.  */
>> +static bool
>> +def_has_ccmode_p (rtx insn)
>> +{
>> +  df_ref *def;
>> +
>> +  for (def = DF_INSN_DEFS (insn); *def; def++)
>> +    {
>> +      enum machine_mode mode = GET_MODE (DF_REF_REG (*def));
>> +
>> +      if (GET_MODE_CLASS (mode) == MODE_CC)
>> +       return true;
>> +    }
>> +
>> +  return false;
>> +}
> 
> FWIW, an alternative might be to test have_regs_of_mode[(int) mode].
> That says whether there are any allocatable (non-fixed) registers
> of the given mode.

While true, I doubt either PPC or MIPS really benefit from moving around registers of CCmode.  Certainly MIPS has no way of easily moving CCmode registers around.  It's a rather complicated reload, that.

I'd be very tempted to simply go with the original patch.


r~
Richard Sandiford - Dec. 22, 2011, 8:53 a.m.
Richard Henderson <rth@redhat.com> writes:
> On 12/20/2011 09:47 AM, Richard Sandiford wrote:
>> Revital Eres <revital.eres@linaro.org> writes:
>>> +/* Return true if one of the definitions in INSN has MODE_CC.  Otherwise
>>> +   return false.  */
>>> +static bool
>>> +def_has_ccmode_p (rtx insn)
>>> +{
>>> +  df_ref *def;
>>> +
>>> +  for (def = DF_INSN_DEFS (insn); *def; def++)
>>> +    {
>>> +      enum machine_mode mode = GET_MODE (DF_REF_REG (*def));
>>> +
>>> +      if (GET_MODE_CLASS (mode) == MODE_CC)
>>> +       return true;
>>> +    }
>>> +
>>> +  return false;
>>> +}
>> 
>> FWIW, an alternative might be to test have_regs_of_mode[(int) mode].
>> That says whether there are any allocatable (non-fixed) registers
>> of the given mode.
>
> While true, I doubt either PPC or MIPS really benefit from moving
> around registers of CCmode.  Certainly MIPS has no way of easily
> moving CCmode registers around.  It's a rather complicated reload,
> that.
>
> I'd be very tempted to simply go with the original patch.

OK, sorry for the noise.

Richard
Ayal Zaks - Jan. 1, 2012, 8:46 a.m.
>The attached patch prevents the creation of reg-moves for definitions
>with MODE_CC and thus solves this ICE.
>
>Currently testing and bootstrap on ppc64-redhat-linux, enabling SMS on
>loops with SC 1.
>
>OK for 4.7 once testing completes?

Yes, thanks for catching this. Shouldn't we prevent creating such
regmoves for (the other case of) intra-loop anti-deps as well?


>> While true, I doubt either PPC or MIPS really benefit from moving
>> around registers of CCmode.

IIRC, PPC has eight 4-bit CR's and supports efficient copying of *one
bit* from one CR to another (via cror), which would require special
regmove handling. Hence I share your doubt.

Ayal.


On Thu, Dec 22, 2011 at 10:53 AM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Henderson <rth@redhat.com> writes:
>> On 12/20/2011 09:47 AM, Richard Sandiford wrote:
>>> Revital Eres <revital.eres@linaro.org> writes:
>>>> +/* Return true if one of the definitions in INSN has MODE_CC.  Otherwise
>>>> +   return false.  */
>>>> +static bool
>>>> +def_has_ccmode_p (rtx insn)
>>>> +{
>>>> +  df_ref *def;
>>>> +
>>>> +  for (def = DF_INSN_DEFS (insn); *def; def++)
>>>> +    {
>>>> +      enum machine_mode mode = GET_MODE (DF_REF_REG (*def));
>>>> +
>>>> +      if (GET_MODE_CLASS (mode) == MODE_CC)
>>>> +       return true;
>>>> +    }
>>>> +
>>>> +  return false;
>>>> +}
>>>
>>> FWIW, an alternative might be to test have_regs_of_mode[(int) mode].
>>> That says whether there are any allocatable (non-fixed) registers
>>> of the given mode.
>>
>> While true, I doubt either PPC or MIPS really benefit from moving
>> around registers of CCmode.  Certainly MIPS has no way of easily
>> moving CCmode registers around.  It's a rather complicated reload,
>> that.
>>
>> I'd be very tempted to simply go with the original patch.
>
> OK, sorry for the noise.
>
> Richard

Patch

Index: ddg.c

===================================================================
--- ddg.c	(revision 182482)

+++ ddg.c	(working copy)

@@ -263,6 +263,23 @@  create_ddg_dep_no_link (ddg_ptr g, ddg_n

     add_edge_to_ddg (g, e);
 }
 
+/* Return true if one of the definitions in INSN has MODE_CC.  Otherwise

+   return false.  */

+static bool

+def_has_ccmode_p (rtx insn)

+{

+  df_ref *def;

+

+  for (def = DF_INSN_DEFS (insn); *def; def++)

+    {

+      enum machine_mode mode = GET_MODE (DF_REF_REG (*def));

+

+      if (GET_MODE_CLASS (mode) == MODE_CC)

+       return true;

+    }

+

+  return false;

+}

 
 /* Given a downwards exposed register def LAST_DEF (which is the last
    definition of that register in the bb), add inter-loop true dependences
@@ -335,7 +352,8 @@  add_cross_iteration_register_deps (ddg_p

           if (DF_REF_ID (last_def) != DF_REF_ID (first_def)
               || !flag_modulo_sched_allow_regmoves
 	      || JUMP_P (use_node->insn)
-              || autoinc_var_is_used_p (DF_REF_INSN (last_def), use_insn))

+              || autoinc_var_is_used_p (DF_REF_INSN (last_def), use_insn)

+	      || def_has_ccmode_p (DF_REF_INSN (last_def)))

             create_ddg_dep_no_link (g, use_node, first_def_node, ANTI_DEP,
                                     REG_DEP, 1);
 
Index: testsuite/gcc.dg/sms-11.c

===================================================================
--- testsuite/gcc.dg/sms-11.c	(revision 0)

+++ testsuite/gcc.dg/sms-11.c	(revision 0)

@@ -0,0 +1,37 @@ 

+/* { dg-do run } */

+/* { dg-options "-O2 -fmodulo-sched -fmodulo-sched-allow-regmoves -fdump-rtl-sms" } */

+

+extern void abort (void);

+

+float out[4][4] = { 6, 6, 7, 5, 6, 7, 5, 5, 6, 4, 4, 4, 6, 2, 3, 4 };

+

+void

+invert (void)

+{

+  int i, j, k = 0, swap;

+  float tmp[4][4] = { 5, 6, 7, 5, 6, 7, 5, 5, 4, 4, 4, 4, 3, 2, 3, 4 };

+

+  for (i = 0; i < 4; i++)

+    {

+      for (j = i + 1; j < 4; j++)

+	if (tmp[j][i] > tmp[i][i])

+	  swap = j;

+

+      if (swap != i)

+	tmp[i][k] = tmp[swap][k];

+    }

+

+  for (i = 0; i < 4; i++)

+    for (j = 0; j < 4; j++)

+      if (tmp[i][j] != out[i][j])

+	abort ();

+}

+

+int

+main ()

+{

+  invert ();

+  return 0;

+}

+

+/* { dg-final { cleanup-rtl-dump "sms" } } */