[PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Kyrill Tkachov]

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

Hi all,

The auto_inc_dec pass can handle 4 types of sequences, described in the comment at the start of auto-inc-dec.c.
In two of those: FORM_PRE_ADD and FORM_POST_ADD the resulting sequence is a move followed by a POST_INC or PRE_INC
memory operation.
In the FORM_POST_ADD case the pass transforms:
            *a
            ...
            b <- a + c

into

            b <- a
            ...
            *(b += c) post


However, the code in attempt_change that compares the costs of the before and after sequences
has an oversight. When calculating the cost of the new sequence it doesn't take into account the cost of the
b <- a move. This patch fixes the calculation by calling seq_cost on the result of the emit_move_insn call
we do to emit that move.

With this patch I've seen less aggressive generation of POST_INC memory instructions on arm, but the post_inc
tests we have in the arm testsuite still pass, so I don't think this kills the usage of those instructions on
arm, just tames them somewhat.

No regressions on SPEC2000 on Cortex-A15.
SPECINT 2006 improves by a bit.

Bootstrapped and tested on arm, aarch64.
Ok for trunk?

Thanks,
Kyrill



2015-10-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * auto-inc-dec.c (insert_move_insn_before): Delete.
     (attempt_change): Remember to cost the simple move in the
     FORM_PRE_ADD and FORM_POST_ADD cases.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: autoincdec-costs.patch --]
[-- Type: text/x-patch; name=autoincdec-costs.patch, Size: 2924 bytes --]

commit 569d1d9b789a38bf4305991d96cbb03d1665e311
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Fri Oct 16 13:46:51 2015 +0100

    [auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

diff --git a/gcc/auto-inc-dec.c b/gcc/auto-inc-dec.c
index 3b9a1f3..af3f8b3 100644
--- a/gcc/auto-inc-dec.c
+++ b/gcc/auto-inc-dec.c
@@ -438,24 +438,6 @@ move_dead_notes (rtx_insn *to_insn, rtx_insn *from_insn, rtx pattern)
     }
 }
 
-
-/* Create a mov insn DEST_REG <- SRC_REG and insert it before
-   NEXT_INSN.  */
-
-static rtx_insn *
-insert_move_insn_before (rtx_insn *next_insn, rtx dest_reg, rtx src_reg)
-{
-  rtx_insn *insns;
-
-  start_sequence ();
-  emit_move_insn (dest_reg, src_reg);
-  insns = get_insns ();
-  end_sequence ();
-  emit_insn_before (insns, next_insn);
-  return insns;
-}
-
-
 /* Change mem_insn.mem_loc so that uses NEW_ADDR which has an
    increment of INC_REG.  To have reached this point, the change is a
    legitimate one from a dataflow point of view.  The only questions
@@ -489,7 +471,23 @@ attempt_change (rtx new_addr, rtx inc_reg)
 
   old_cost = (set_src_cost (mem, mode, speed)
 	      + set_rtx_cost (PATTERN (inc_insn.insn), speed));
-  new_cost = set_src_cost (mem_tmp, mode, speed);
+
+  int new_mem_cost = set_src_cost (mem_tmp, mode, speed);
+  int new_mov_cost = 0;
+
+  /* In the FORM_PRE_ADD and FORM_POST_ADD cases we emit an extra move
+     whose cost we should account for.  */
+  if (inc_insn.form == FORM_PRE_ADD
+      || inc_insn.form == FORM_POST_ADD)
+    {
+      start_sequence ();
+      emit_move_insn (inc_insn.reg_res, inc_insn.reg0);
+      mov_insn = get_insns ();
+      end_sequence ();
+      new_mov_cost = seq_cost (mov_insn, speed);
+    }
+
+  new_cost = new_mem_cost + new_mov_cost;
 
   /* The first item of business is to see if this is profitable.  */
   if (old_cost < new_cost)
@@ -521,8 +519,8 @@ attempt_change (rtx new_addr, rtx inc_reg)
       /* Replace the addition with a move.  Do it at the location of
 	 the addition since the operand of the addition may change
 	 before the memory reference.  */
-      mov_insn = insert_move_insn_before (inc_insn.insn,
-					  inc_insn.reg_res, inc_insn.reg0);
+      gcc_assert (mov_insn);
+      emit_insn_before (mov_insn, inc_insn.insn);
       move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0);
 
       regno = REGNO (inc_insn.reg_res);
@@ -547,8 +545,8 @@ attempt_change (rtx new_addr, rtx inc_reg)
       break;
 
     case FORM_POST_ADD:
-      mov_insn = insert_move_insn_before (mem_insn.insn,
-					  inc_insn.reg_res, inc_insn.reg0);
+      gcc_assert (mov_insn);
+      emit_insn_before (mov_insn, mem_insn.insn);
       move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0);
 
       /* Do not move anything to the mov insn because the instruction

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Bernd Schmidt]

On 10/26/2015 11:40 AM, Kyrill Tkachov wrote:
> In the FORM_POST_ADD case the pass transforms:
>             *a
>             ...
>             b <- a + c
>
> into
>
>             b <- a
>             ...
>             *(b += c) post
>
>
> However, the code in attempt_change that compares the costs of the
> before and after sequences
> has an oversight. When calculating the cost of the new sequence it
> doesn't take into account the cost of the
> b <- a move. This patch fixes the calculation by calling seq_cost on the
> result of the emit_move_insn call
> we do to emit that move.

But isn't that balanced by the fact that it doesn't seem to take into 
account the gain of removing the inc_insn either? So I think this can't 
be right.

> +      new_mov_cost = seq_cost (mov_insn, speed);
> +    }
> +
> +  new_cost = new_mem_cost + new_mov_cost;

Here I'd just replace the first line with
   new_cost += seq_cost (...)
and lose the extra variable.

I seem to recall Richard had a rewrite of all the autoinc code. I wonder 
what happened to that?


Bernd

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Bernd Schmidt]

On 10/26/2015 12:12 PM, Bernd Schmidt wrote:
>
> But isn't that balanced by the fact that it doesn't seem to take into
> account the gain of removing the inc_insn either? So I think this can't
> be right.

Argh, misread the code. The patch is OK with the change I suggested.


Bernd

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Kyrill Tkachov]

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


On 26/10/15 11:28, Bernd Schmidt wrote:
> On 10/26/2015 12:12 PM, Bernd Schmidt wrote:
>>
>> But isn't that balanced by the fact that it doesn't seem to take into
>> account the gain of removing the inc_insn either? So I think this can't
>> be right.
>
> Argh, misread the code. The patch is OK with the change I suggested.
>

Thanks!
Here's what I committed with r229344.

Kyrill

2015-10-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * auto-inc-dec.c (insert_move_insn_before): Delete.
     (attempt_change): Remember to cost the simple move in the
     FORM_PRE_ADD and FORM_POST_ADD cases.

>
> Bernd
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: autoincdec-costs.patch --]
[-- Type: text/x-patch; name=autoincdec-costs.patch, Size: 2796 bytes --]

commit cc7c4748eac1f9d59ceb5393132c098aba99765d
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Fri Oct 16 13:46:51 2015 +0100

    [auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

diff --git a/gcc/auto-inc-dec.c b/gcc/auto-inc-dec.c
index e003b13..9f7c8e0 100644
--- a/gcc/auto-inc-dec.c
+++ b/gcc/auto-inc-dec.c
@@ -439,24 +439,6 @@ move_dead_notes (rtx_insn *to_insn, rtx_insn *from_insn, rtx pattern)
     }
 }
 
-
-/* Create a mov insn DEST_REG <- SRC_REG and insert it before
-   NEXT_INSN.  */
-
-static rtx_insn *
-insert_move_insn_before (rtx_insn *next_insn, rtx dest_reg, rtx src_reg)
-{
-  rtx_insn *insns;
-
-  start_sequence ();
-  emit_move_insn (dest_reg, src_reg);
-  insns = get_insns ();
-  end_sequence ();
-  emit_insn_before (insns, next_insn);
-  return insns;
-}
-
-
 /* Change mem_insn.mem_loc so that uses NEW_ADDR which has an
    increment of INC_REG.  To have reached this point, the change is a
    legitimate one from a dataflow point of view.  The only questions
@@ -490,8 +472,21 @@ attempt_change (rtx new_addr, rtx inc_reg)
 
   old_cost = (set_src_cost (mem, mode, speed)
 	      + set_rtx_cost (PATTERN (inc_insn.insn), speed));
+
   new_cost = set_src_cost (mem_tmp, mode, speed);
 
+  /* In the FORM_PRE_ADD and FORM_POST_ADD cases we emit an extra move
+     whose cost we should account for.  */
+  if (inc_insn.form == FORM_PRE_ADD
+      || inc_insn.form == FORM_POST_ADD)
+    {
+      start_sequence ();
+      emit_move_insn (inc_insn.reg_res, inc_insn.reg0);
+      mov_insn = get_insns ();
+      end_sequence ();
+      new_cost += seq_cost (mov_insn, speed);
+    }
+
   /* The first item of business is to see if this is profitable.  */
   if (old_cost < new_cost)
     {
@@ -522,8 +517,8 @@ attempt_change (rtx new_addr, rtx inc_reg)
       /* Replace the addition with a move.  Do it at the location of
 	 the addition since the operand of the addition may change
 	 before the memory reference.  */
-      mov_insn = insert_move_insn_before (inc_insn.insn,
-					  inc_insn.reg_res, inc_insn.reg0);
+      gcc_assert (mov_insn);
+      emit_insn_before (mov_insn, inc_insn.insn);
       move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0);
 
       regno = REGNO (inc_insn.reg_res);
@@ -548,8 +543,8 @@ attempt_change (rtx new_addr, rtx inc_reg)
       break;
 
     case FORM_POST_ADD:
-      mov_insn = insert_move_insn_before (mem_insn.insn,
-					  inc_insn.reg_res, inc_insn.reg0);
+      gcc_assert (mov_insn);
+      emit_insn_before (mov_insn, mem_insn.insn);
       move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0);
 
       /* Do not move anything to the mov insn because the instruction

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Oleg Endo]

On Mon, 2015-10-26 at 12:12 +0100, Bernd Schmidt wrote:
> On 10/26/2015 11:40 AM, Kyrill Tkachov wrote:
> > In the FORM_POST_ADD case the pass transforms:
> >             *a
> >             ...
> >             b <- a + c
> > 
> > into
> > 
> >             b <- a
> >             ...
> >             *(b += c) post
> > 
> > 
> > However, the code in attempt_change that compares the costs of the
> > before and after sequences
> > has an oversight. When calculating the cost of the new sequence it
> > doesn't take into account the cost of the
> > b <- a move. This patch fixes the calculation by calling seq_cost
> > on the
> > result of the emit_move_insn call
> > we do to emit that move.
> 
> But isn't that balanced by the fact that it doesn't seem to take into
> account the gain of removing the inc_insn either? So I think this
> can't 
> be right.
> 
> > +      new_mov_cost = seq_cost (mov_insn, speed);
> > +    }
> > +
> > +  new_cost = new_mem_cost + new_mov_cost;
> 
> Here I'd just replace the first line with
>    new_cost += seq_cost (...)
> and lose the extra variable.
> 
> I seem to recall Richard had a rewrite of all the autoinc code. I
> wonder 
> what happened to that?

BTW there's been another recent attempt at replacing auto-inc-dec with
a more generic addressing mode selection (AMS) pass.  It tries to take
into account costs of individual addressing modes for each mem access
and also combinations of address register modifications and addressing
modes.  The initial version is for SH only and still requires some work
before it can be merged into mainline.  I hope that I can make it for
GCC 6...

Cheers,
Oleg

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Richard Sandiford]

Bernd Schmidt <bschmidt@redhat.com> writes:
> I seem to recall Richard had a rewrite of all the autoinc code. I wonder 
> what happened to that?

Although it produced more autoincs, it didn't really improve performance
that much on the targets I was looking at at the time.

I'm afraid the patch is long lost now, and would probably be in an
uncertain copyright situation anyway.

Thanks,
Richard

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Jeff Law]

On 10/26/2015 10:07 AM, Richard Sandiford wrote:
> Bernd Schmidt <bschmidt@redhat.com> writes:
>> I seem to recall Richard had a rewrite of all the autoinc code. I wonder
>> what happened to that?
>
> Although it produced more autoincs, it didn't really improve performance
> that much on the targets I was looking at at the time.
>
> I'm afraid the patch is long lost now, and would probably be in an
> uncertain copyright situation anyway.
Yup.  I wouldn't want to untangle that mess of legal opinions.

Out of curiosity, what was the basic premise behind what you did to get 
more autoincs?
jeff

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Christophe Lyon]

Hi Kyrill,

On 26 October 2015 at 12:52, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>
> On 26/10/15 11:28, Bernd Schmidt wrote:
>>
>> On 10/26/2015 12:12 PM, Bernd Schmidt wrote:
>>>
>>>
>>> But isn't that balanced by the fact that it doesn't seem to take into
>>> account the gain of removing the inc_insn either? So I think this can't
>>> be right.
>>
>>
>> Argh, misread the code. The patch is OK with the change I suggested.
>>
>
> Thanks!
> Here's what I committed with r229344.
>
Since this commit, I've noticed:

FAIL: gcc.target/arm/lp1243022.c scan-rtl-dump subreg2 "REG_INC"
with --target arm-none-eabi --with-thumb --with-cpu=cortex-a9

as well as ICEs:
  gcc.target/aarch64/advsimd-intrinsics/vldX.c   -O2  (internal compiler error)
  gcc.target/aarch64/advsimd-intrinsics/vldX.c   -O2 -flto
-fno-use-linker-plugin -flto-partition=none  (internal compiler error)

with --target arm-none-linux-gnueabihf --with-thumb
--with-cpu=cortex-a15 --with-fpu=neon-vfpv4
and --target arm-none-linux-gnueabihf --with-thumb
--with-cpu=cortex-a57 --with-fpu=crypto-neon-fp-armv8

See for a more synthetic view:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/229344/report-build-info.html

Can you have a look?

Thanks,

Christophe.

> Kyrill
>
> 2015-10-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     * auto-inc-dec.c (insert_move_insn_before): Delete.
>     (attempt_change): Remember to cost the simple move in the
>     FORM_PRE_ADD and FORM_POST_ADD cases.
>
>>
>> Bernd
>>
>

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Kyrill Tkachov]

On 28/10/15 17:23, Christophe Lyon wrote:
> Hi Kyrill,

Hi Christophe,

>
> On 26 October 2015 at 12:52, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>> On 26/10/15 11:28, Bernd Schmidt wrote:
>>> On 10/26/2015 12:12 PM, Bernd Schmidt wrote:
>>>>
>>>> But isn't that balanced by the fact that it doesn't seem to take into
>>>> account the gain of removing the inc_insn either? So I think this can't
>>>> be right.
>>>
>>> Argh, misread the code. The patch is OK with the change I suggested.
>>>
>> Thanks!
>> Here's what I committed with r229344.
>>
> Since this commit, I've noticed:
>
> FAIL: gcc.target/arm/lp1243022.c scan-rtl-dump subreg2 "REG_INC"
> with --target arm-none-eabi --with-thumb --with-cpu=cortex-a9

I think this is a testcase issue, I'll look into updating it separately.

> as well as ICEs:
>    gcc.target/aarch64/advsimd-intrinsics/vldX.c   -O2  (internal compiler error)
>    gcc.target/aarch64/advsimd-intrinsics/vldX.c   -O2 -flto
> -fno-use-linker-plugin -flto-partition=none  (internal compiler error)
>
> with --target arm-none-linux-gnueabihf --with-thumb
> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
> and --target arm-none-linux-gnueabihf --with-thumb
> --with-cpu=cortex-a57 --with-fpu=crypto-neon-fp-armv8
>
> See for a more synthetic view:
> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/229344/report-build-info.html
>
> Can you have a look?

The ICE backtrace is:
0x7a4494 change_address_1
         $SRC/gcc/emit-rtl.c:2132
0x7a7453 adjust_address_1(rtx_def*, machine_mode, long, int, int, int, long)
         $SRC/gcc/emit-rtl.c:2270
0xab792d gen_lowpart_general(machine_mode, rtx_def*)
         $SRC/gcc/rtlhooks.c:90
0xfd721d gen_split_47(rtx_insn*, rtx_def**)
         $SRC/gcc/config/arm/arm.md:4336
0x12473f2 split_11
         $SRC/gcc/config/arm/arm.md:4331
0x12473f2 split_insns(rtx_def*, rtx_insn*)
         $SRC/gcc/config/arm/sync.md:361
0x7af30e try_split(rtx_def*, rtx_insn*, int)
         $SRC/gcc/emit-rtl.c:3664
0xa53dc5 split_insn
         $SRC/gcc/recog.c:2874
0xa5ccf5 split_all_insns()
         $SRC/gcc/recog.c:2964
0xa5cde3 rest_of_handle_split_after_reload
         $SRC/gcc/recog.c:3900
0xa5cde3 execute
         $SRC/gcc/recog.c:3929


Looks like a latent issue exposed by my patch.
Could you please file a BZ entry if get the chance?

Thanks,
Kyrill


> Thanks,
>
> Christophe.
>
>> Kyrill
>>
>> 2015-10-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      * auto-inc-dec.c (insert_move_insn_before): Delete.
>>      (attempt_change): Remember to cost the simple move in the
>>      FORM_PRE_ADD and FORM_POST_ADD cases.
>>
>>> Bernd
>>>

Re: [PATCH][auto-inc-dec.c] Account for cost of move operation in FORM_PRE_ADD and FORM_POST_ADD cases

[Kyrill Tkachov]

On 28/10/15 17:45, Kyrill Tkachov wrote:
>
> On 28/10/15 17:23, Christophe Lyon wrote:
>> Hi Kyrill,
>
> Hi Christophe,
>
>>
>> On 26 October 2015 at 12:52, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>>> On 26/10/15 11:28, Bernd Schmidt wrote:
>>>> On 10/26/2015 12:12 PM, Bernd Schmidt wrote:
>>>>>
>>>>> But isn't that balanced by the fact that it doesn't seem to take into
>>>>> account the gain of removing the inc_insn either? So I think this can't
>>>>> be right.
>>>>
>>>> Argh, misread the code. The patch is OK with the change I suggested.
>>>>
>>> Thanks!
>>> Here's what I committed with r229344.
>>>
>> Since this commit, I've noticed:
>>
>> FAIL: gcc.target/arm/lp1243022.c scan-rtl-dump subreg2 "REG_INC"
>> with --target arm-none-eabi --with-thumb --with-cpu=cortex-a9
>
> I think this is a testcase issue, I'll look into updating it separately.
>
>> as well as ICEs:
>>    gcc.target/aarch64/advsimd-intrinsics/vldX.c   -O2  (internal compiler error)
>>    gcc.target/aarch64/advsimd-intrinsics/vldX.c   -O2 -flto
>> -fno-use-linker-plugin -flto-partition=none  (internal compiler error)
>>
>> with --target arm-none-linux-gnueabihf --with-thumb
>> --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
>> and --target arm-none-linux-gnueabihf --with-thumb
>> --with-cpu=cortex-a57 --with-fpu=crypto-neon-fp-armv8
>>
>> See for a more synthetic view:
>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/229344/report-build-info.html
>>
>> Can you have a look?
>
> The ICE backtrace is:
> 0x7a4494 change_address_1
>         $SRC/gcc/emit-rtl.c:2132
> 0x7a7453 adjust_address_1(rtx_def*, machine_mode, long, int, int, int, long)
>         $SRC/gcc/emit-rtl.c:2270
> 0xab792d gen_lowpart_general(machine_mode, rtx_def*)
>         $SRC/gcc/rtlhooks.c:90
> 0xfd721d gen_split_47(rtx_insn*, rtx_def**)
>         $SRC/gcc/config/arm/arm.md:4336
> 0x12473f2 split_11
>         $SRC/gcc/config/arm/arm.md:4331
> 0x12473f2 split_insns(rtx_def*, rtx_insn*)
>         $SRC/gcc/config/arm/sync.md:361
> 0x7af30e try_split(rtx_def*, rtx_insn*, int)
>         $SRC/gcc/emit-rtl.c:3664
> 0xa53dc5 split_insn
>         $SRC/gcc/recog.c:2874
> 0xa5ccf5 split_all_insns()
>         $SRC/gcc/recog.c:2964
> 0xa5cde3 rest_of_handle_split_after_reload
>         $SRC/gcc/recog.c:3900
> 0xa5cde3 execute
>         $SRC/gcc/recog.c:3929
>
>
> Looks like a latent issue exposed by my patch.
> Could you please file a BZ entry if get the chance?
>

I've filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68149 for this one.

Kyrill

> Thanks,
> Kyrill
>
>
>> Thanks,
>>
>> Christophe.
>>
>>> Kyrill
>>>
>>> 2015-10-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      * auto-inc-dec.c (insert_move_insn_before): Delete.
>>>      (attempt_change): Remember to cost the simple move in the
>>>      FORM_PRE_ADD and FORM_POST_ADD cases.
>>>
>>>> Bernd
>>>>
>