[PATCH] LRA: Fix caller-save store/restore instruction for large mode

[PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Kito Cheng]

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

Hi Vladimir:
  This patch has a discusses with you in May 2014, this patch is about
the caller-save register store and restore instruction generation, the
current LRA implementation will miss caller-save store/restore
instruction if need one more instruction.

You said you will investigate for this on IRC, so I don't send the
patch last year, however ARM guys seem got this problem too, so I
think it's time to send this patch :)

ChangeLog

2015-01-05  Kito Cheng  <kito@0xlab.org>

        * lra-constraints.c (split_reg): Fix caller-save store/restore
instruction generation.

[-- Attachment #2: 0001-Fix-caller-save-store-restore-instruction-for-large-.patch --]
[-- Type: text/x-patch, Size: 1360 bytes --]

From 434dc294cf3f2267eed89d27b34c53c079e2b25a Mon Sep 17 00:00:00 2001
From: Kito Cheng <kito@andestech.com>
Date: Thu, 29 May 2014 23:53:23 +0800
Subject: [PATCH] Fix caller-save store/restore instruction for large mode in
 lra

 - The original code assume store/restore will always have only
   one insn, it's will fail if you split in move pattern!!!
---
 gcc/lra-constraints.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 382281c..4dcf553 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4918,9 +4918,8 @@ split_reg (bool before_p, int original_regno, rtx_insn *insn,
       reg_renumber[REGNO (new_reg)] = hard_regno;
     }
   save = emit_spill_move (true, new_reg, original_reg);
-  if (NEXT_INSN (save) != NULL_RTX)
+  if (NEXT_INSN (save) != NULL_RTX && !call_save_p)
     {
-      lra_assert (! call_save_p);
       if (lra_dump_file != NULL)
 	{
 	  fprintf
@@ -4934,9 +4933,8 @@ split_reg (bool before_p, int original_regno, rtx_insn *insn,
       return false;
     }
   restore = emit_spill_move (false, new_reg, original_reg);
-  if (NEXT_INSN (restore) != NULL_RTX)
+  if (NEXT_INSN (restore) != NULL_RTX && !call_save_p)
     {
-      lra_assert (! call_save_p);
       if (lra_dump_file != NULL)
 	{
 	  fprintf (lra_dump_file,
-- 
1.7.6

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Bin.Cheng]

On Mon, Jan 5, 2015 at 3:44 PM, Kito Cheng <kito.cheng@gmail.com> wrote:
> Hi Vladimir:
>   This patch has a discusses with you in May 2014, this patch is about
> the caller-save register store and restore instruction generation, the
> current LRA implementation will miss caller-save store/restore
> instruction if need one more instruction.
>
> You said you will investigate for this on IRC, so I don't send the
> patch last year, however ARM guys seem got this problem too, so I
> think it's time to send this patch :)
>
> ChangeLog
>
> 2015-01-05  Kito Cheng  <kito@0xlab.org>
>
>         * lra-constraints.c (split_reg): Fix caller-save store/restore
> instruction generation.
Hi,
Thanks for saving my work, I was going to send this exact patch.
Note that we have PR64348 tracking the issue now.

Thanks,
bin

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Jeff Law]

On 01/05/15 00:44, Kito Cheng wrote:
> Hi Vladimir:
>    This patch has a discusses with you in May 2014, this patch is about
> the caller-save register store and restore instruction generation, the
> current LRA implementation will miss caller-save store/restore
> instruction if need one more instruction.
>
> You said you will investigate for this on IRC, so I don't send the
> patch last year, however ARM guys seem got this problem too, so I
> think it's time to send this patch :)
>
> ChangeLog
>
> 2015-01-05  Kito Cheng  <kito@0xlab.org>
>
>          * lra-constraints.c (split_reg): Fix caller-save store/restore
> instruction generation.
Please reference PR64348 in the ChangeLog entry.

Please include a testcase if there isn't one in the regression suite 
already.

Please indicate what platform this patch was bootstrapped and regression 
tested on.

The dumping code immediately after the assert you removed has code like 
this in both cases:



          fprintf (lra_dump_file,
                    "    Rejecting split %d->%d "
                    "resulting in > 2 %s restore insns:\n",
                    original_regno, REGNO (new_reg), call_save_p ? 
"call" : "");

Testing call_save_p here won't make any sense after your patch.

I'll let Vlad chime in on the correctness of allowing multi register 
saves/restores in this code.

Jeff

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Vladimir Makarov]

On 2015-01-05 12:31 PM, Jeff Law wrote:
> On 01/05/15 00:44, Kito Cheng wrote:
>> Hi Vladimir:
>>    This patch has a discusses with you in May 2014, this patch is about
>> the caller-save register store and restore instruction generation, the
>> current LRA implementation will miss caller-save store/restore
>> instruction if need one more instruction.
>>
>> You said you will investigate for this on IRC, so I don't send the
>> patch last year, however ARM guys seem got this problem too, so I
>> think it's time to send this patch :)
>>
>> ChangeLog
>>
>> 2015-01-05  Kito Cheng  <kito@0xlab.org>
>>
>>          * lra-constraints.c (split_reg): Fix caller-save store/restore
>> instruction generation.
> Please reference PR64348 in the ChangeLog entry.
>
> Please include a testcase if there isn't one in the regression suite 
> already.
>
> Please indicate what platform this patch was bootstrapped and 
> regression tested on.
>
> The dumping code immediately after the assert you removed has code 
> like this in both cases:
>
>
>
>          fprintf (lra_dump_file,
>                    "    Rejecting split %d->%d "
>                    "resulting in > 2 %s restore insns:\n",
>                    original_regno, REGNO (new_reg), call_save_p ? 
> "call" : "");
>
> Testing call_save_p here won't make any sense after your patch.
>
> I'll let Vlad chime in on the correctness of allowing multi register 
> saves/restores in this code.
>
The solution itself is ok.  Prohibiting generation of more one insn was 
intended for inheritance only as inheritance transformation can be 
undone when the inheritance pseudo does not get a hard register. Undoing 
multi-register splitting is difficult and also such splitting is 
doubtedly profitable.

Splitting for save/restore is never undone.  So it is ok for this case 
to generate multi-register saves/restores.

Kito, Jeff wrote reasonable changes for the patch.  Please, do them and 
you can commit the patch.

Thanks.

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Bin.Cheng]

On Tue, Jan 6, 2015 at 7:36 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:
>
> On 2015-01-05 12:31 PM, Jeff Law wrote:
>>
>> On 01/05/15 00:44, Kito Cheng wrote:
>>>
>>> Hi Vladimir:
>>>    This patch has a discusses with you in May 2014, this patch is about
>>> the caller-save register store and restore instruction generation, the
>>> current LRA implementation will miss caller-save store/restore
>>> instruction if need one more instruction.
>>>
>>> You said you will investigate for this on IRC, so I don't send the
>>> patch last year, however ARM guys seem got this problem too, so I
>>> think it's time to send this patch :)
>>>
>>> ChangeLog
>>>
>>> 2015-01-05  Kito Cheng  <kito@0xlab.org>
>>>
>>>          * lra-constraints.c (split_reg): Fix caller-save store/restore
>>> instruction generation.
>>
>> Please reference PR64348 in the ChangeLog entry.
>>
>> Please include a testcase if there isn't one in the regression suite
>> already.
>>
>> Please indicate what platform this patch was bootstrapped and regression
>> tested on.
>>
>> The dumping code immediately after the assert you removed has code like
>> this in both cases:
>>
>>
>>
>>          fprintf (lra_dump_file,
>>                    "    Rejecting split %d->%d "
>>                    "resulting in > 2 %s restore insns:\n",
>>                    original_regno, REGNO (new_reg), call_save_p ? "call" :
>> "");
>>
>> Testing call_save_p here won't make any sense after your patch.
>>
>> I'll let Vlad chime in on the correctness of allowing multi register
>> saves/restores in this code.
>>
> The solution itself is ok.  Prohibiting generation of more one insn was
> intended for inheritance only as inheritance transformation can be undone
> when the inheritance pseudo does not get a hard register. Undoing
> multi-register splitting is difficult and also such splitting is doubtedly
> profitable.
>
> Splitting for save/restore is never undone.  So it is ok for this case to
> generate multi-register saves/restores.
>
> Kito, Jeff wrote reasonable changes for the patch.  Please, do them and you
> can commit the patch.

Hi Vlad,
As for this specific case in PR64348, dump IR crossing function call
is as below:

  430: [sfp:SI-0x30]=r989:TI#0
  432: [r1706:SI+0x4]=r989:TI#4
  434: [r1706:SI+0x8]=r989:TI#8
  436: [r1706:SI+0xc]=r989:TI#12
  441: r0:DI=call [`__aeabi_idivmod'] argc:0
      REG_UNUSED r0:SI
      REG_CALL_DECL `__aeabi_idivmod'
      REG_EH_REGION 0xffffffff80000000
  437: r1007:SI=sign_extend(r989:TI#0)
      REG_DEAD r989:TI

Save/restore are introduced because of use of r989:#0 in insn 437.
Register r989 is TImode register and assigned to r2/r3/r4/r5.  I can
think about two possible improvements to LRA splitter.  1) According
to ARM EABI, only r2/r3 need to be saved/restored; 2) In this case, we
only need to save/restore r989:TI#0, which is r2.  In fact, it has
already been saved to stack in insn 430, what we need is to restore
r989:TI#0 after function call.

What do you think about these?

Thanks,
bin

>
> Thanks.
>

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Vladimir Makarov]

On 2015-01-05 8:40 PM, Bin.Cheng wrote:
> On Tue, Jan 6, 2015 at 7:36 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:
>> On 2015-01-05 12:31 PM, Jeff Law wrote:
>>> On 01/05/15 00:44, Kito Cheng wrote:
>>>> Hi Vladimir:
>>>>     This patch has a discusses with you in May 2014, this patch is about
>>>> the caller-save register store and restore instruction generation, the
>>>> current LRA implementation will miss caller-save store/restore
>>>> instruction if need one more instruction.
>>>>
>>>> You said you will investigate for this on IRC, so I don't send the
>>>> patch last year, however ARM guys seem got this problem too, so I
>>>> think it's time to send this patch :)
>>>>
>>>> ChangeLog
>>>>
>>>> 2015-01-05  Kito Cheng  <kito@0xlab.org>
>>>>
>>>>           * lra-constraints.c (split_reg): Fix caller-save store/restore
>>>> instruction generation.
>>> Please reference PR64348 in the ChangeLog entry.
>>>
>>> Please include a testcase if there isn't one in the regression suite
>>> already.
>>>
>>> Please indicate what platform this patch was bootstrapped and regression
>>> tested on.
>>>
>>> The dumping code immediately after the assert you removed has code like
>>> this in both cases:
>>>
>>>
>>>
>>>           fprintf (lra_dump_file,
>>>                     "    Rejecting split %d->%d "
>>>                     "resulting in > 2 %s restore insns:\n",
>>>                     original_regno, REGNO (new_reg), call_save_p ? "call" :
>>> "");
>>>
>>> Testing call_save_p here won't make any sense after your patch.
>>>
>>> I'll let Vlad chime in on the correctness of allowing multi register
>>> saves/restores in this code.
>>>
>> The solution itself is ok.  Prohibiting generation of more one insn was
>> intended for inheritance only as inheritance transformation can be undone
>> when the inheritance pseudo does not get a hard register. Undoing
>> multi-register splitting is difficult and also such splitting is doubtedly
>> profitable.
>>
>> Splitting for save/restore is never undone.  So it is ok for this case to
>> generate multi-register saves/restores.
>>
>> Kito, Jeff wrote reasonable changes for the patch.  Please, do them and you
>> can commit the patch.
> Hi Vlad,
> As for this specific case in PR64348, dump IR crossing function call
> is as below:
>
>    430: [sfp:SI-0x30]=r989:TI#0
>    432: [r1706:SI+0x4]=r989:TI#4
>    434: [r1706:SI+0x8]=r989:TI#8
>    436: [r1706:SI+0xc]=r989:TI#12
>    441: r0:DI=call [`__aeabi_idivmod'] argc:0
>        REG_UNUSED r0:SI
>        REG_CALL_DECL `__aeabi_idivmod'
>        REG_EH_REGION 0xffffffff80000000
>    437: r1007:SI=sign_extend(r989:TI#0)
>        REG_DEAD r989:TI
>
> Save/restore are introduced because of use of r989:#0 in insn 437.
> Register r989 is TImode register and assigned to r2/r3/r4/r5.  I can
> think about two possible improvements to LRA splitter.  1) According
> to ARM EABI, only r2/r3 need to be saved/restored; 2) In this case, we
> only need to save/restore r989:TI#0, which is r2.  In fact, it has
> already been saved to stack in insn 430, what we need is to restore
> r989:TI#0 after function call.
>
> What do you think about these?
>
>
Thanks for the interesting case.

It would be nice to implement this but it is hard.   For saving only r2 
we need live range analysis on sub-register level which is complicated 
(even IRA has such code only at most for 2-registers pseudos).  For 
reusing memory from insn 430 is complicated too if it is not equiv memory.

May be adding hard registers explicitly to this code could help 
(save/restore splitting is done only on the 1st LRA sub-pass; after that 
pseudos can get a call-clobbered hard register only if it does not 
intersect calls; if pseudo r989 is spilled on later LRA sub-passes we 
could remove code involving explicit subregisters). I'll think about this.

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Kito Cheng]

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

Hi Jeff:

It's updated patch,bootstrapped and run regression tested on arm-eabi,
arm-none-linux-uclibcgnueabi, x86_64-unknown-linux-gnu and nds32le-elf
without introducing regression.

Thanks for your review :)

2015-01-07  Kito Cheng  <kito@0xlab.org>

        PR target/64348
        * lra-constraints.c (split_reg): Fix caller-save store/restore
instruction generation.

[-- Attachment #2: 0001-Fix-caller-save-store-restore-instruction-for-large-.patch --]
[-- Type: text/x-patch, Size: 5851 bytes --]

From b6a697ee5697b84082f6e619f4a435e6c750d695 Mon Sep 17 00:00:00 2001
From: Kito Cheng <kito@andestech.com>
Date: Thu, 29 May 2014 23:53:23 +0800
Subject: [PATCH] Fix caller-save store/restore instruction for large mode in
 lra

 - The original code assume store/restore will always have only
   one insn, it's will fail if you split in move pattern!!!
---
 gcc/lra-constraints.c                  | 14 +++---
 gcc/testsuite/gcc.target/arm/pr64348.c | 89 ++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/arm/pr64348.c

diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 96b10a1..f8bc12f 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4918,15 +4918,14 @@ split_reg (bool before_p, int original_regno, rtx_insn *insn,
       reg_renumber[REGNO (new_reg)] = hard_regno;
     }
   save = emit_spill_move (true, new_reg, original_reg);
-  if (NEXT_INSN (save) != NULL_RTX)
+  if (NEXT_INSN (save) != NULL_RTX && !call_save_p)
     {
-      lra_assert (! call_save_p);
       if (lra_dump_file != NULL)
 	{
 	  fprintf
 	    (lra_dump_file,
-	     "	  Rejecting split %d->%d resulting in > 2 %s save insns:\n",
-	     original_regno, REGNO (new_reg), call_save_p ? "call" : "");
+	     "	  Rejecting split %d->%d resulting in > 2 save insns:\n",
+	     original_regno, REGNO (new_reg));
 	  dump_rtl_slim (lra_dump_file, save, NULL, -1, 0);
 	  fprintf (lra_dump_file,
 		   "	))))))))))))))))))))))))))))))))))))))))))))))))\n");
@@ -4934,15 +4933,14 @@ split_reg (bool before_p, int original_regno, rtx_insn *insn,
       return false;
     }
   restore = emit_spill_move (false, new_reg, original_reg);
-  if (NEXT_INSN (restore) != NULL_RTX)
+  if (NEXT_INSN (restore) != NULL_RTX && !call_save_p)
     {
-      lra_assert (! call_save_p);
       if (lra_dump_file != NULL)
 	{
 	  fprintf (lra_dump_file,
 		   "	Rejecting split %d->%d "
-		   "resulting in > 2 %s restore insns:\n",
-		   original_regno, REGNO (new_reg), call_save_p ? "call" : "");
+		   "resulting in > 2 restore insns:\n",
+		   original_regno, REGNO (new_reg));
 	  dump_rtl_slim (lra_dump_file, restore, NULL, -1, 0);
 	  fprintf (lra_dump_file,
 		   "	))))))))))))))))))))))))))))))))))))))))))))))))\n");
diff --git a/gcc/testsuite/gcc.target/arm/pr64348.c b/gcc/testsuite/gcc.target/arm/pr64348.c
new file mode 100644
index 0000000..b949f35
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64348.c
@@ -0,0 +1,89 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fPIC -marm -mcpu=cortex-a8" } */
+
+#define vector(elcount, type)  \
+__attribute__((vector_size((elcount)*sizeof(type)))) type
+
+#define vidx(type, vec, idx) (*((type *) &(vec) + idx))
+
+#define operl(a, b, op) (a op b)
+#define operr(a, b, op) (b op a)
+
+#define check(type, count, vec0, vec1, num, op, lr) \
+do {\
+    int __i; \
+    for (__i = 0; __i < count; __i++) {\
+        if (vidx (type, vec1, __i) != oper##lr (num, vidx (type, vec0, __i), op)) \
+            __builtin_abort (); \
+    }\
+} while (0)
+
+#define veccompare(type, count, v0, v1) \
+do {\
+    int __i; \
+    for (__i = 0; __i < count; __i++) { \
+        if (vidx (type, v0, __i) != vidx (type, v1, __i)) \
+            __builtin_abort (); \
+    } \
+} while (0)
+
+volatile int one = 1;
+
+int main (int argc, char *argv[]) {
+#define fvec_2 (vector(4, float)){2., 2., 2., 2.}
+#define dvec_2 (vector(2, double)){2., 2.}
+
+
+    vector(8, short) v0 = {one, 1, 2, 3, 4, 5, 6, 7};
+    vector(8, short) v1;
+
+    vector(4, float) f0 = {1., 2., 3., 4.};
+    vector(4, float) f1, f2;
+
+    vector(2, double) d0 = {1., 2.};
+    vector(2, double) d1, d2;
+
+
+
+    v1 = 2 + v0;   check (short, 8, v0, v1, 2, +, l);
+    v1 = 2 - v0;   check (short, 8, v0, v1, 2, -, l);
+    v1 = 2 * v0;   check (short, 8, v0, v1, 2, *, l);
+    v1 = 2 / v0;   check (short, 8, v0, v1, 2, /, l);
+    v1 = 2 % v0;   check (short, 8, v0, v1, 2, %, l);
+    v1 = 2 ^ v0;   check (short, 8, v0, v1, 2, ^, l);
+    v1 = 2 & v0;   check (short, 8, v0, v1, 2, &, l);
+    v1 = 2 | v0;   check (short, 8, v0, v1, 2, |, l);
+    v1 = 2 << v0;   check (short, 8, v0, v1, 2, <<, l);
+    v1 = 2 >> v0;   check (short, 8, v0, v1, 2, >>, l);
+
+    v1 = v0 + 2;   check (short, 8, v0, v1, 2, +, r);
+    v1 = v0 - 2;   check (short, 8, v0, v1, 2, -, r);
+    v1 = v0 * 2;   check (short, 8, v0, v1, 2, *, r);
+    v1 = v0 / 2;   check (short, 8, v0, v1, 2, /, r);
+    v1 = v0 % 2;   check (short, 8, v0, v1, 2, %, r);
+    v1 = v0 ^ 2;   check (short, 8, v0, v1, 2, ^, r);
+    v1 = v0 & 2;   check (short, 8, v0, v1, 2, &, r);
+    v1 = v0 | 2;   check (short, 8, v0, v1, 2, |, r);
+
+    f1 = 2. + f0;  f2 = fvec_2 + f0; veccompare (float, 4, f1, f2);
+    f1 = 2. - f0;  f2 = fvec_2 - f0; veccompare (float, 4, f1, f2);
+    f1 = 2. * f0;  f2 = fvec_2 * f0; veccompare (float, 4, f1, f2);
+    f1 = 2. / f0;  f2 = fvec_2 / f0; veccompare (float, 4, f1, f2);
+
+    f1 = f0 + 2.;  f2 = f0 + fvec_2; veccompare (float, 4, f1, f2);
+    f1 = f0 - 2.;  f2 = f0 - fvec_2; veccompare (float, 4, f1, f2);
+    f1 = f0 * 2.;  f2 = f0 * fvec_2; veccompare (float, 4, f1, f2);
+    f1 = f0 / 2.;  f2 = f0 / fvec_2; veccompare (float, 4, f1, f2);
+
+    d1 = 2. + d0;  d2 = dvec_2 + d0; veccompare (double, 2, d1, d2);
+    d1 = 2. - d0;  d2 = dvec_2 - d0; veccompare (double, 2, d1, d2);
+    d1 = 2. * d0;  d2 = dvec_2 * d0; veccompare (double, 2, d1, d2);
+    d1 = 2. / d0;  d2 = dvec_2 / d0; veccompare (double, 2, d1, d2);
+
+    d1 = d0 + 2.;  d2 = d0 + dvec_2; veccompare (double, 2, d1, d2);
+    d1 = d0 - 2.;  d2 = d0 - dvec_2; veccompare (double, 2, d1, d2);
+    d1 = d0 * 2.;  d2 = d0 * dvec_2; veccompare (double, 2, d1, d2);
+    d1 = d0 / 2.;  d2 = d0 / dvec_2; veccompare (double, 2, d1, d2);
+
+    return 0;
+}
-- 
2.1.0

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Bin.Cheng]

On Wed, Jan 7, 2015 at 4:03 PM, Kito Cheng <kito.cheng@gmail.com> wrote:
> Hi Jeff:
>
> It's updated patch,bootstrapped and run regression tested on arm-eabi,
> arm-none-linux-uclibcgnueabi, x86_64-unknown-linux-gnu and nds32le-elf
> without introducing regression.
>
> Thanks for your review :)
>
> 2015-01-07  Kito Cheng  <kito@0xlab.org>
>
>         PR target/64348
>         * lra-constraints.c (split_reg): Fix caller-save store/restore
> instruction generation.

Thanks for fixing the issue.
The PR is against existing testcase failure
gcc.c-torture/execute/scal-to-vec1.c.  Unless we can create a new
case, there is no need to include same case twice I think?  Or we can
mention the PR number in the original test case?

Thanks,
bin

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Kito Cheng]

Hi Bin:

It's 2 more line than gcc.c-torture/execute/scal-to-vec1.c since it's
need specific compilation
flag and specific target to reproduce this issue,
and it's can't reproduce by normal testing flow with
arm-*-linux-gnueabi (due to lack -fPIC flag),
so I prefer duplicate this case into gcc.target/arm/ :)

/* { dg-do compile } */
/* { dg-options "-O3 -fPIC -marm -mcpu=cortex-a8" } */

On Wed, Jan 7, 2015 at 4:50 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, Jan 7, 2015 at 4:03 PM, Kito Cheng <kito.cheng@gmail.com> wrote:
>> Hi Jeff:
>>
>> It's updated patch,bootstrapped and run regression tested on arm-eabi,
>> arm-none-linux-uclibcgnueabi, x86_64-unknown-linux-gnu and nds32le-elf
>> without introducing regression.
>>
>> Thanks for your review :)
>>
>> 2015-01-07  Kito Cheng  <kito@0xlab.org>
>>
>>         PR target/64348
>>         * lra-constraints.c (split_reg): Fix caller-save store/restore
>> instruction generation.
>
> Thanks for fixing the issue.
> The PR is against existing testcase failure
> gcc.c-torture/execute/scal-to-vec1.c.  Unless we can create a new
> case, there is no need to include same case twice I think?  Or we can
> mention the PR number in the original test case?
>
> Thanks,
> bin

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Bin.Cheng]

On Wed, Jan 7, 2015 at 8:28 PM, Kito Cheng <kito.cheng@gmail.com> wrote:
> Hi Bin:
>
> It's 2 more line than gcc.c-torture/execute/scal-to-vec1.c since it's
> need specific compilation
> flag and specific target to reproduce this issue,
> and it's can't reproduce by normal testing flow with
> arm-*-linux-gnueabi (due to lack -fPIC flag),
> so I prefer duplicate this case into gcc.target/arm/ :)
>
> /* { dg-do compile } */
> /* { dg-options "-O3 -fPIC -marm -mcpu=cortex-a8" } */
Not really, we generally want to avoid cpu related options in testcase
since it introduces conflict option failures when testing against
specific processor, e.g. testing against Cortex-M profile processors.

Thanks,
bin

>
> On Wed, Jan 7, 2015 at 4:50 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> On Wed, Jan 7, 2015 at 4:03 PM, Kito Cheng <kito.cheng@gmail.com> wrote:
>>> Hi Jeff:
>>>
>>> It's updated patch,bootstrapped and run regression tested on arm-eabi,
>>> arm-none-linux-uclibcgnueabi, x86_64-unknown-linux-gnu and nds32le-elf
>>> without introducing regression.
>>>
>>> Thanks for your review :)
>>>
>>> 2015-01-07  Kito Cheng  <kito@0xlab.org>
>>>
>>>         PR target/64348
>>>         * lra-constraints.c (split_reg): Fix caller-save store/restore
>>> instruction generation.
>>
>> Thanks for fixing the issue.
>> The PR is against existing testcase failure
>> gcc.c-torture/execute/scal-to-vec1.c.  Unless we can create a new
>> case, there is no need to include same case twice I think?  Or we can
>> mention the PR number in the original test case?
>>
>> Thanks,
>> bin

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Kito Cheng]

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

Hi Jeff:

After discussion with Bin, he prefer just use
gcc.c-torture/execute/scal-to-vec1.c
instead of introduce new one, do you have any further comment on this patch?

On Thu, Jan 8, 2015 at 9:29 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, Jan 7, 2015 at 8:28 PM, Kito Cheng <kito.cheng@gmail.com> wrote:
>> Hi Bin:
>>
>> It's 2 more line than gcc.c-torture/execute/scal-to-vec1.c since it's
>> need specific compilation
>> flag and specific target to reproduce this issue,
>> and it's can't reproduce by normal testing flow with
>> arm-*-linux-gnueabi (due to lack -fPIC flag),
>> so I prefer duplicate this case into gcc.target/arm/ :)
>>
>> /* { dg-do compile } */
>> /* { dg-options "-O3 -fPIC -marm -mcpu=cortex-a8" } */
> Not really, we generally want to avoid cpu related options in testcase
> since it introduces conflict option failures when testing against
> specific processor, e.g. testing against Cortex-M profile processors.
>
> Thanks,
> bin
>
>>
>> On Wed, Jan 7, 2015 at 4:50 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>> On Wed, Jan 7, 2015 at 4:03 PM, Kito Cheng <kito.cheng@gmail.com> wrote:
>>>> Hi Jeff:
>>>>
>>>> It's updated patch,bootstrapped and run regression tested on arm-eabi,
>>>> arm-none-linux-uclibcgnueabi, x86_64-unknown-linux-gnu and nds32le-elf
>>>> without introducing regression.
>>>>
>>>> Thanks for your review :)
>>>>
>>>> 2015-01-07  Kito Cheng  <kito@0xlab.org>
>>>>
>>>>         PR target/64348
>>>>         * lra-constraints.c (split_reg): Fix caller-save store/restore
>>>> instruction generation.
>>>
>>> Thanks for fixing the issue.
>>> The PR is against existing testcase failure
>>> gcc.c-torture/execute/scal-to-vec1.c.  Unless we can create a new
>>> case, there is no need to include same case twice I think?  Or we can
>>> mention the PR number in the original test case?
>>>
>>> Thanks,
>>> bin

[-- Attachment #2: 0001-Fix-caller-save-store-restore-instruction-for-large-.patch --]
[-- Type: text/x-patch, Size: 2172 bytes --]

From 98f58cca618a99e282c7add2ecbe59002c867c60 Mon Sep 17 00:00:00 2001
From: Kito Cheng <kito@andestech.com>
Date: Thu, 29 May 2014 23:53:23 +0800
Subject: [PATCH] Fix caller-save store/restore instruction for large mode in
 lra

 - The original code assume store/restore will always have only
   one insn, it's will fail if you split in move pattern!!!
---
 gcc/lra-constraints.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 96b10a1..f8bc12f 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4918,15 +4918,14 @@ split_reg (bool before_p, int original_regno, rtx_insn *insn,
       reg_renumber[REGNO (new_reg)] = hard_regno;
     }
   save = emit_spill_move (true, new_reg, original_reg);
-  if (NEXT_INSN (save) != NULL_RTX)
+  if (NEXT_INSN (save) != NULL_RTX && !call_save_p)
     {
-      lra_assert (! call_save_p);
       if (lra_dump_file != NULL)
 	{
 	  fprintf
 	    (lra_dump_file,
-	     "	  Rejecting split %d->%d resulting in > 2 %s save insns:\n",
-	     original_regno, REGNO (new_reg), call_save_p ? "call" : "");
+	     "	  Rejecting split %d->%d resulting in > 2 save insns:\n",
+	     original_regno, REGNO (new_reg));
 	  dump_rtl_slim (lra_dump_file, save, NULL, -1, 0);
 	  fprintf (lra_dump_file,
 		   "	))))))))))))))))))))))))))))))))))))))))))))))))\n");
@@ -4934,15 +4933,14 @@ split_reg (bool before_p, int original_regno, rtx_insn *insn,
       return false;
     }
   restore = emit_spill_move (false, new_reg, original_reg);
-  if (NEXT_INSN (restore) != NULL_RTX)
+  if (NEXT_INSN (restore) != NULL_RTX && !call_save_p)
     {
-      lra_assert (! call_save_p);
       if (lra_dump_file != NULL)
 	{
 	  fprintf (lra_dump_file,
 		   "	Rejecting split %d->%d "
-		   "resulting in > 2 %s restore insns:\n",
-		   original_regno, REGNO (new_reg), call_save_p ? "call" : "");
+		   "resulting in > 2 restore insns:\n",
+		   original_regno, REGNO (new_reg));
 	  dump_rtl_slim (lra_dump_file, restore, NULL, -1, 0);
 	  fprintf (lra_dump_file,
 		   "	))))))))))))))))))))))))))))))))))))))))))))))))\n");
-- 
1.8.3.1.549.g1f3a412

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Jeff Law]

On 01/08/15 08:58, Kito Cheng wrote:
> Hi Jeff:
>
> After discussion with Bin, he prefer just use
> gcc.c-torture/execute/scal-to-vec1.c
> instead of introduce new one, do you have any further comment on this patch?
If you can use the existing test, then that's good for me.  I believe 
that was the last question and that everything has been approved, right?

jeff

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Jeff Law]

On 01/08/15 08:58, Kito Cheng wrote:
> Hi Jeff:
>
> After discussion with Bin, he prefer just use
> gcc.c-torture/execute/scal-to-vec1.c
> instead of introduce new one, do you have any further comment on this patch?
Ah, if there's an existing test, then we certainly don't need a new one.

Jeff

Re: [PATCH] LRA: Fix caller-save store/restore instruction for large mode

[Bin.Cheng]

On Fri, Jan 9, 2015 at 6:03 AM, Jeff Law <law@redhat.com> wrote:
> On 01/08/15 08:58, Kito Cheng wrote:
>>
>> Hi Jeff:
>>
>> After discussion with Bin, he prefer just use
>> gcc.c-torture/execute/scal-to-vec1.c
>> instead of introduce new one, do you have any further comment on this
>> patch?
>
> Ah, if there's an existing test, then we certainly don't need a new one.
>

Hi, according to the review comments, I applied the new version patch
on behalf of Kito (because of no write access) as revision 219375.

Thanks,
bin