Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Terry Guo]

Hi there,

Is this one ok to trunk?

BR,
Terry

On Wed, Apr 15, 2015 at 6:45 PM, Hale Wang <hale.wang@arm.com> wrote:
> Ping for trunk?
>
> Hale
>
>> -----Original Message-----
>> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
>> Sent: Friday, February 27, 2015 4:04 AM
>> To: Terry Guo
>> Cc: Segher Boessenkool; Richard Sandiford; GCC Patches; Hale Wang
>> Subject: Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the
> insns
>> if a volatile register is contained.
>>
>> Terry Guo <flameroc@gmail.com> writes:
>> > On Thu, Feb 26, 2015 at 1:55 PM, Segher Boessenkool
>> > <segher@kernel.crashing.org> wrote:
>> >> On Tue, Feb 17, 2015 at 11:39:34AM +0800, Terry Guo wrote:
>> >>> On Sun, Feb 15, 2015 at 7:35 PM, Segher Boessenkool
>> >>> <segher@kernel.crashing.org> wrote:
>> >>> > Hi Terry,
>> >>> >
>> >>> > I still think this is stage1 material.
>> >>> >
>> >>> >> + /* Don't combine if dest contains a user specified register and
>> >>> >> i3 contains
>> >>> >> + ASM_OPERANDS, because the user specified register (same with
>> >>> >> dest) in i3
>> >>> >> +     would be replaced by the src of insn which might be different
>> with
>> >>> >> +     the user's expectation.  */
>> >>> >
>> >>> > "Do not eliminate a register asm in an asm input" or similar?
>> >>> > Text explaining why REG_USERVAR_P && HARD_REGISTER_P works
>> here
>> >>> > would be good to have, too.
>> >>
>> >>> diff --git a/gcc/combine.c b/gcc/combine.c index f779117..aeb2854
>> >>> 100644
>> >>> --- a/gcc/combine.c
>> >>> +++ b/gcc/combine.c
>> >>> @@ -1779,7 +1779,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3,
>> >>> rtx_insn *pred ATTRIBUTE_UNUSED,  {
>> >>>    int i;
>> >>>    const_rtx set = 0;
>> >>> -  rtx src, dest;
>> >>> +  rtx src, dest, asm_op;
>> >>>    rtx_insn *p;
>> >>>  #ifdef AUTO_INC_DEC
>> >>>    rtx link;
>> >>> @@ -1914,6 +1914,14 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3,
>> rtx_insn *pred ATTRIBUTE_UNUSED,
>> >>>    set = expand_field_assignment (set);
>> >>>    src = SET_SRC (set), dest = SET_DEST (set);
>> >>>
>> >>> +  /* Use REG_USERVAR_P and HARD_REGISTER_P to check whether
>> DEST is a user
>> >>> +     specified register, and do not eliminate such register if it is
> in an
>> >>> +     asm input because we may end up with something different with
>> user's
>> >>> +     expectation.  */
>> >>
>> >> That doesn't explain why this will hit (almost) only on register asms.
>> >> The user's expectation doesn't matter that much either: GCC would
>> >> violate its own documentation / promises, that matters more ;-)
>> >>
>> >>> +  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P
>> (dest)
>> >>> +      && ((asm_op = extract_asm_operands (PATTERN (i3))) != NULL))
>> >>
>> >> You do not need the temporary variable, nor the != 0 or the extra
>> >> parens; just write
>> >>
>> >>      && extract_asm_operands (PATTERN (i3))
>> >>
>> >> Cheers,
>> >>
>> >>
>> >> Segher
>> >
>> > Thanks for comments. Patch is updated now. Please review again.
>>
>> Looks good to me FWIW.
>>
>> Thanks,
>> Richard
>
>
>

Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Segher Boessenkool]

On Tue, Apr 21, 2015 at 09:39:16AM +0800, Terry Guo wrote:
> Is this one ok to trunk?

Probably, if you send the patch + changelog entry :-)

Did you fix the comment?  REG_USERVAR_P and HARD_REGISTER_P can be
set for more than just register asm.


Segher

Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Terry Guo]

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

On Tue, Apr 21, 2015 at 11:03 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Tue, Apr 21, 2015 at 09:39:16AM +0800, Terry Guo wrote:
>> Is this one ok to trunk?
>
> Probably, if you send the patch + changelog entry :-)
>
> Did you fix the comment?  REG_USERVAR_P and HARD_REGISTER_P can be
> set for more than just register asm.
>
>
> Segher

Sorry for missing the patch. I believe that I addressed your patch.
Please review it again to make sure my understanding is correct. The
patch is attached and here is the URL to it
https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01593.html. The
ChangeLog:

gcc/ChangeLog:
2015-04-21  Terry Guo  <terry.guo@arm.com>

       PR rtl-optimization/64818
       * combine.c (can_combine_p): Don't combine if DEST is a user-specified
       register.

gcc/testsuite/ChangeLog:

2015-04-21  Terry Guo  <terry.guo@arm.com>

       PR rtl-optimization/64818
       * gcc.target/arm/pr64818.c: New.

[-- Attachment #2: pr64818-combine-user-specified-register.patch-5 --]
[-- Type: application/octet-stream, Size: 1691 bytes --]

diff --git a/gcc/combine.c b/gcc/combine.c
index f779117..d3c1b87 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1914,6 +1914,14 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
   set = expand_field_assignment (set);
   src = SET_SRC (set), dest = SET_DEST (set);
 
+  /* Use REG_USERVAR_P and HARD_REGISTER_P to check whether DEST is a user
+     specified register, and do not eliminate such register if it is in an
+     asm input.  Otherwise if allow such elimination, we may break the
+     register asm usage defined in GCC manual.  */
+  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
+      && extract_asm_operands (PATTERN (i3)))
+    return 0;
+
   /* Don't eliminate a store in the stack pointer.  */
   if (dest == stack_pointer_rtx
       /* Don't combine with an insn that sets a register to itself if it has
diff --git a/gcc/testsuite/gcc.target/arm/pr64818.c b/gcc/testsuite/gcc.target/arm/pr64818.c
new file mode 100644
index 0000000..bddd846
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64818.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+char temp[16];
+extern int foo1 (void);
+
+void foo (void)
+{
+  int i;
+  int len;
+
+  while (1)
+  {
+    len = foo1 ();
+    register int a asm ("r0") = 5;
+    register char *b asm ("r1") = temp;
+    register int c asm ("r2") = len;
+    asm volatile ("mov %[r0], %[r0]\n  mov %[r1], %[r1]\n  mov %[r2], %[r2]\n"
+		   : "+m"(*b)
+		   : [r0]"r"(a), [r1]"r"(b), [r2]"r"(c));
+
+    for (i = 0; i < len; i++)
+    {
+      if (temp[i] == 10)
+      return;
+    }
+  }
+}
+
+/* { dg-final { scan-assembler "\[\\t \]+mov\ r1,\ r1" } } */

Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Segher Boessenkool]

On Tue, Apr 21, 2015 at 03:13:38PM +0800, Terry Guo wrote:
> > Did you fix the comment?  REG_USERVAR_P and HARD_REGISTER_P can be
> > set for more than just register asm.
> 
> Sorry for missing the patch. I believe that I addressed your patch.
> Please review it again to make sure my understanding is correct.

> +  /* Use REG_USERVAR_P and HARD_REGISTER_P to check whether DEST is a user
> +     specified register, and do not eliminate such register if it is in an
> +     asm input.  Otherwise if allow such elimination, we may break the
> +     register asm usage defined in GCC manual.  */
> +  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
> +      && extract_asm_operands (PATTERN (i3)))
> +    return 0;

The "to check whether DEST is a user-specified register" part is not
correct; this check can for example also match for function arguments
(which are hard regs) that were combined into any "normal" user var.
I don't see how we would do a better check, and disallowing combination
in this case is harmless (or even good); but the comment is misleading.


Segher

Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Terry Guo]

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

On Wed, Apr 22, 2015 at 9:44 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Tue, Apr 21, 2015 at 03:13:38PM +0800, Terry Guo wrote:
>> > Did you fix the comment?  REG_USERVAR_P and HARD_REGISTER_P can be
>> > set for more than just register asm.
>>
>> Sorry for missing the patch. I believe that I addressed your patch.
>> Please review it again to make sure my understanding is correct.
>
>> +  /* Use REG_USERVAR_P and HARD_REGISTER_P to check whether DEST is a user
>> +     specified register, and do not eliminate such register if it is in an
>> +     asm input.  Otherwise if allow such elimination, we may break the
>> +     register asm usage defined in GCC manual.  */
>> +  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
>> +      && extract_asm_operands (PATTERN (i3)))
>> +    return 0;
>
> The "to check whether DEST is a user-specified register" part is not
> correct; this check can for example also match for function arguments
> (which are hard regs) that were combined into any "normal" user var.
> I don't see how we would do a better check, and disallowing combination
> in this case is harmless (or even good); but the comment is misleading.
>
>
> Segher

Thanks for reviewing. Patch is updated per you suggestion. The
ChangeLog is also updated as below:

gcc/ChangeLog:
2015-04-22 Hale Wang <hale.wang@arm.com>
                    Terry Guo  <terry.guo@arm.com>

       PR rtl-optimization/64818
       * combine.c (can_combine_p): Don't combine user-specified register if
       it is in an asm input.

gcc/testsuite/ChangeLog:
2015-04-22 Hale Wang <hale.wang@arm.com>
                    Terry Guo  <terry.guo@arm.com>

       PR rtl-optimization/64818
       * gcc.target/arm/pr64818.c: New.

[-- Attachment #2: pr64818-combine-user-specified-register-v6.txt --]
[-- Type: text/plain, Size: 1684 bytes --]

diff --git a/gcc/combine.c b/gcc/combine.c
index 6f0007a..6cd55dd 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1910,6 +1910,15 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
   set = expand_field_assignment (set);
   src = SET_SRC (set), dest = SET_DEST (set);
 
+  /* Do not eliminate user-specified register if it is in an
+     asm input because we may break the register asm usage defined
+     in GCC manual if allow to do so.
+     Be aware that this may cover more cases than we expect but this
+     should be harmless.  */
+  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
+      && extract_asm_operands (PATTERN (i3)))
+    return 0;
+
   /* Don't eliminate a store in the stack pointer.  */
   if (dest == stack_pointer_rtx
       /* Don't combine with an insn that sets a register to itself if it has
diff --git a/gcc/testsuite/gcc.target/arm/pr64818.c b/gcc/testsuite/gcc.target/arm/pr64818.c
new file mode 100644
index 0000000..bddd846
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64818.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+char temp[16];
+extern int foo1 (void);
+
+void foo (void)
+{
+  int i;
+  int len;
+
+  while (1)
+  {
+    len = foo1 ();
+    register int a asm ("r0") = 5;
+    register char *b asm ("r1") = temp;
+    register int c asm ("r2") = len;
+    asm volatile ("mov %[r0], %[r0]\n  mov %[r1], %[r1]\n  mov %[r2], %[r2]\n"
+		   : "+m"(*b)
+		   : [r0]"r"(a), [r1]"r"(b), [r2]"r"(c));
+
+    for (i = 0; i < len; i++)
+    {
+      if (temp[i] == 10)
+      return;
+    }
+  }
+}
+
+/* { dg-final { scan-assembler "\[\\t \]+mov\ r1,\ r1" } } */

Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Segher Boessenkool]

On Wed, Apr 22, 2015 at 10:21:43AM +0800, Terry Guo wrote:
> gcc/ChangeLog:
> 2015-04-22 Hale Wang <hale.wang@arm.com>
>                     Terry Guo  <terry.guo@arm.com>
> 
>        PR rtl-optimization/64818
>        * combine.c (can_combine_p): Don't combine user-specified register if
>        it is in an asm input.
> 
> gcc/testsuite/ChangeLog:
> 2015-04-22 Hale Wang <hale.wang@arm.com>
>                     Terry Guo  <terry.guo@arm.com>
> 
>        PR rtl-optimization/64818
>        * gcc.target/arm/pr64818.c: New.

This is okay for trunk, if it has been bootstrapped and regression tested.

Thanks,


Segher

Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Terry Guo]

On Wed, Apr 22, 2015 at 10:30 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Wed, Apr 22, 2015 at 10:21:43AM +0800, Terry Guo wrote:
>> gcc/ChangeLog:
>> 2015-04-22 Hale Wang <hale.wang@arm.com>
>>                     Terry Guo  <terry.guo@arm.com>
>>
>>        PR rtl-optimization/64818
>>        * combine.c (can_combine_p): Don't combine user-specified register if
>>        it is in an asm input.
>>
>> gcc/testsuite/ChangeLog:
>> 2015-04-22 Hale Wang <hale.wang@arm.com>
>>                     Terry Guo  <terry.guo@arm.com>
>>
>>        PR rtl-optimization/64818
>>        * gcc.target/arm/pr64818.c: New.
>
> This is okay for trunk, if it has been bootstrapped and regression tested.
>
> Thanks,
>
>
> Segher

Thanks Segher. The patch is tested with bootstrap and regression test
for x86_64. No problem found. Committed as revision 222306.

BR,
Terry