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

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

[Hale Wang]

Ping?

> -----Original Message-----
> From: Hale Wang [mailto:hale.wang@arm.com]
> Sent: Thursday, January 29, 2015 9:58 AM
> To: Hale Wang; 'Segher Boessenkool'
> Cc: GCC Patches
> Subject: RE: [PATCH] [gcc, combine] PR46164: Don't combine the insns if a
> volatile register is contained.
> 
> Hi Segher,
> 
> I have updated the patch as you suggested. Both the patch and the
> changelog are attached.
> 
> By the way, the test case provided by Tim Pambor in PR46164 was a
different
> bug with PR46164. So I resubmitted the bug in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64818.
> And this patch is just used to fix this bug. Is it OK for you?
> 
> Thanks,
> Hale
> 
> gcc/ChangeLog:
> 2015-01-27  Segher Boessenkool  <segher@kernel.crashing.org>
> 	    Hale Wang  <hale.wang@arm.com>
> 
> 	PR rtl-optimization/64818
> 	* combine.c (can_combine_p): Don't combine the insn if
> 	the dest of insn is a user specified register.
> 
> gcc/testsuit/ChangeLog:
> 2015-01-27  Segher Boessenkool  <segher@kernel.crashing.org>
> 	    Hale Wang  <hale.wang@arm.com>
> 
> 	PR rtl-optimization/64818
> 	* gcc.target/arm/pr64818.c: New test.
> 
> 
> diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -1904,6 +1904,12 @@ 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);
> 
> +  /* Don't combine if dest contains a user specified register, 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.
> + */  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P
> (dest))
> +    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" } } */
> 
> 
> > > On Tue, Jan 27, 2015 at 11:49:55AM +0800, Hale Wang wrote:
> > >
> > > Hi Hale,
> > > > You are correct. Just "-O1" reproduces this problem.
> > > > However it's a combine bug which is related to the combing user
> > > > specified register into inline-asm.
> > >
> > > Yes, it is.  But the registers the testcase uses exist on any ARM
> > > version
> > there
> > > is as far as I know, so not specifying specific model and ABI should
> > > give
> > wider
> > > test coverage (if anyone actually builds and/or tests more than the
> > default,
> > > of course :-) )
> > >
> > > > > Could you try this patch please?
> > > >
> > > > Your patch rejected the combine 98+43, that's correct.
> > >
> > > Excellent, thanks for testing.
> > >
> > > > However, Jakub
> > > > pointed out that preventing that to be combined would be a serious
> > > > regression on code quality.
> > >
> > > I know; I needed to think of some good way to detect register
> > > variables
> > (they
> > > aren't marked specially in RTL).  I think I found one, for combine
> > > that
> > is; if we
> > > need to detect it in other passes too, we probably need to put
> > > another
> > flag
> > > on it, or something.
> > >
> > > > Andrew Pinski suggested: can_combine_p would reject combing into
> > > > an inline-asm to prevent this issue. And I have updated the patch.
> > > > What do you think about this change?
> > >
> > > That will regress combining anything else into an asm.  It will
> > > disallow combining asms _at all_, if we really wanted that we should
> > > simply not
> > build
> > > LOG_LINKS for them.  But it hurts optimisation (for simple "r"
> > > constraints
> > it is
> > > not a real problem, RA should take care of it, but for anything else
> > > it
> > is).
> > >
> > > Updated patch below.  A user variable that is also a hard register
> > > can
> > only
> > > happen in a few cases: 1) a register variable, the case we are
> > > after;
> > > 2)
> > an
> > > argument for the current function that was propagated into a user
> > > variable (something combine should not do at all, it hinders good
> > > register
> > allocation,
> > > but it does anyway on most targets).
> > >
> > > Do you want to take this or shall I?  This is not a regression, so
> > > it
> > probably
> > > should wait for stage1 :-(
> > >
> >
> > Your solution is very good. I will test this patch locally and send
> > out the result ASAP.
> > Thanks,
> >
> > Hale
> >
> > >
> > > Segher
> > >

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

[Richard Sandiford]

"Hale Wang" <hale.wang@arm.com> writes:
> Ping?
>
>> -----Original Message-----
>> From: Hale Wang [mailto:hale.wang@arm.com]
>> Sent: Thursday, January 29, 2015 9:58 AM
>> To: Hale Wang; 'Segher Boessenkool'
>> Cc: GCC Patches
>> Subject: RE: [PATCH] [gcc, combine] PR46164: Don't combine the insns if a
>> volatile register is contained.
>> 
>> Hi Segher,
>> 
>> I have updated the patch as you suggested. Both the patch and the
>> changelog are attached.
>> 
>> By the way, the test case provided by Tim Pambor in PR46164 was a
> different
>> bug with PR46164. So I resubmitted the bug in
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64818.
>> And this patch is just used to fix this bug. Is it OK for you?
>> 
>> Thanks,
>> Hale
>> 
>> gcc/ChangeLog:
>> 2015-01-27  Segher Boessenkool  <segher@kernel.crashing.org>
>> 	    Hale Wang  <hale.wang@arm.com>
>> 
>> 	PR rtl-optimization/64818
>> 	* combine.c (can_combine_p): Don't combine the insn if
>> 	the dest of insn is a user specified register.
>> 
>> gcc/testsuit/ChangeLog:
>> 2015-01-27  Segher Boessenkool  <segher@kernel.crashing.org>
>> 	    Hale Wang  <hale.wang@arm.com>
>> 
>> 	PR rtl-optimization/64818
>> 	* gcc.target/arm/pr64818.c: New test.
>> 
>> 
>> diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2 100644
>> --- a/gcc/combine.c
>> +++ b/gcc/combine.c
>> @@ -1904,6 +1904,12 @@ 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);
>> 
>> +  /* Don't combine if dest contains a user specified register, 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.
>> + */  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P
>> (dest))
>> +    return 0;

I suppose this is similar to Andrew's comment, but I think the rule
is that it's invalid to replace a REG_USERVAR_P operand in an inline asm.
Outside of an inline asm we make no guarantee about whether something is
stored in a particular register or not.

So IMO we should be checking whether either INSN or I3 is an asm as well
as the above.

Thanks,
Richard

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

[Segher Boessenkool]

On Thu, Feb 12, 2015 at 03:54:21PM +0000, Richard Sandiford wrote:
> "Hale Wang" <hale.wang@arm.com> writes:
> > Ping?

It's not a regression (or is it?), so it is not appropriate for stage4.


> >> diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2 100644
> >> --- a/gcc/combine.c
> >> +++ b/gcc/combine.c
> >> @@ -1904,6 +1904,12 @@ 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);
> >> 
> >> +  /* Don't combine if dest contains a user specified register, 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.
> >> + */  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P
> >> (dest))
> >> +    return 0;
> 
> I suppose this is similar to Andrew's comment, but I think the rule
> is that it's invalid to replace a REG_USERVAR_P operand in an inline asm.

Why not?  You probably mean register asm, not all user variables?

> Outside of an inline asm we make no guarantee about whether something is
> stored in a particular register or not.
> 
> So IMO we should be checking whether either INSN or I3 is an asm as well
> as the above.

[ INSN can never be an asm, that is already refused by can_combine_p. ]

We do not guarantee things will end up in the specified reg (except for asm),
but will it hurt to leave things in the reg the user said it should be in, even
if we do not guarantee this behaviour?


Segher

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

[Jeff Law]

On 02/12/15 15:15, Segher Boessenkool wrote:
> On Thu, Feb 12, 2015 at 03:54:21PM +0000, Richard Sandiford wrote:
>> "Hale Wang" <hale.wang@arm.com> writes:
>>> Ping?
>
> It's not a regression (or is it?), so it is not appropriate for stage4.
That's the big question, of course.

> [ INSN can never be an asm, that is already refused by can_combine_p. ]
>
> We do not guarantee things will end up in the specified reg (except for asm),
> but will it hurt to leave things in the reg the user said it should be in, even
> if we do not guarantee this behaviour?
I doubt it could hurt except in extreme corner cases where the value 
gets used later in some insn where the user register was inappropriate. 
  That will cause a spill to copy from the user register to a scratch of 
the appropriate type.

That's, of course, if I'm reading this correctly...

jeff

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

[Hale Wang]

> -----Original Message-----
> From: Segher Boessenkool [mailto:segher@kernel.crashing.org]
> Sent: Friday, February 13, 2015 6:16 AM
> To: Hale Wang; 'GCC Patches'; Richard Sandiford
> Subject: Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the
insns
> if a volatile register is contained.
> 
> On Thu, Feb 12, 2015 at 03:54:21PM +0000, Richard Sandiford wrote:
> > "Hale Wang" <hale.wang@arm.com> writes:
> > > Ping?
> 
> It's not a regression (or is it?), so it is not appropriate for stage4.
> 
> 
> > >> diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2
> > >> 100644
> > >> --- a/gcc/combine.c
> > >> +++ b/gcc/combine.c
> > >> @@ -1904,6 +1904,12 @@ 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);
> > >>
> > >> +  /* Don't combine if dest contains a user specified register,
> > >> + 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.
> > >> + */  if (REG_P (dest) && REG_USERVAR_P (dest) &&
> HARD_REGISTER_P
> > >> (dest))
> > >> +    return 0;
> >
> > I suppose this is similar to Andrew's comment, but I think the rule is
> > that it's invalid to replace a REG_USERVAR_P operand in an inline asm.
> 
> Why not?  You probably mean register asm, not all user variables?
> 
> > Outside of an inline asm we make no guarantee about whether something
> > is stored in a particular register or not.
> >
> > So IMO we should be checking whether either INSN or I3 is an asm as
> > well as the above.
> 
> [ INSN can never be an asm, that is already refused by can_combine_p. ]
> 

Indeed. If INSN is an asm operand, it's already refused by can_combine_p.

Hale.

> We do not guarantee things will end up in the specified reg (except for
asm),
> but will it hurt to leave things in the reg the user said it should be in,
even if
> we do not guarantee this behaviour?
> 
> 
> Segher

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

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Feb 12, 2015 at 03:54:21PM +0000, Richard Sandiford wrote:
>> "Hale Wang" <hale.wang@arm.com> writes:
>> > Ping?
>
> It's not a regression (or is it?), so it is not appropriate for stage4.
>
>
>> >> diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2 100644
>> >> --- a/gcc/combine.c
>> >> +++ b/gcc/combine.c
>> >> @@ -1904,6 +1904,12 @@ 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);
>> >> 
>> >> +  /* Don't combine if dest contains a user specified register, 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.
>> >> + */  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P
>> >> (dest))
>> >> +    return 0;
>> 
>> I suppose this is similar to Andrew's comment, but I think the rule
>> is that it's invalid to replace a REG_USERVAR_P operand in an inline asm.
>
> Why not?  You probably mean register asm, not all user variables?

Yeah, meant hard REG_USERVAR_P, sorry, as for the patch.

>> Outside of an inline asm we make no guarantee about whether something is
>> stored in a particular register or not.
>> 
>> So IMO we should be checking whether either INSN or I3 is an asm as well
>> as the above.
>
> [ INSN can never be an asm, that is already refused by can_combine_p. ]
>
> We do not guarantee things will end up in the specified reg (except for asm),
> but will it hurt to leave things in the reg the user said it should be in, even
> if we do not guarantee this behaviour?

Whether it does not, making the test unnecessarily wide is at best only
going to paper over problems elsewhere.  I really think we should test
for i3 being an asm.

Thanks,
Richard

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

[Terry Guo]

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

On Fri, Feb 13, 2015 at 5:06 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> On Thu, Feb 12, 2015 at 03:54:21PM +0000, Richard Sandiford wrote:
>>> "Hale Wang" <hale.wang@arm.com> writes:
>>> > Ping?
>>
>> It's not a regression (or is it?), so it is not appropriate for stage4.
>>
>>
>>> >> diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2 100644
>>> >> --- a/gcc/combine.c
>>> >> +++ b/gcc/combine.c
>>> >> @@ -1904,6 +1904,12 @@ 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);
>>> >>
>>> >> +  /* Don't combine if dest contains a user specified register, 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.
>>> >> + */  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P
>>> >> (dest))
>>> >> +    return 0;
>>>
>>> I suppose this is similar to Andrew's comment, but I think the rule
>>> is that it's invalid to replace a REG_USERVAR_P operand in an inline asm.
>>
>> Why not?  You probably mean register asm, not all user variables?
>
> Yeah, meant hard REG_USERVAR_P, sorry, as for the patch.
>
>>> Outside of an inline asm we make no guarantee about whether something is
>>> stored in a particular register or not.
>>>
>>> So IMO we should be checking whether either INSN or I3 is an asm as well
>>> as the above.
>>
>> [ INSN can never be an asm, that is already refused by can_combine_p. ]
>>
>> We do not guarantee things will end up in the specified reg (except for asm),
>> but will it hurt to leave things in the reg the user said it should be in, even
>> if we do not guarantee this behaviour?
>
> Whether it does not, making the test unnecessarily wide is at best only
> going to paper over problems elsewhere.  I really think we should test
> for i3 being an asm.
>
> Thanks,
> Richard

Thanks for reviewing. Hale wants me to continue his work because he
will be in holiday in next ten days. The check of asm is added. Is
this one OK?

BR,
Terry

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

diff --git a/gcc/combine.c b/gcc/combine.c
index f779117..c51d0b4 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1914,6 +1914,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);
 
+  /* 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.  */
+  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
+      && (GET_CODE (PATTERN (i3)) == SET
+	  && GET_CODE (SET_SRC (PATTERN (i3))) == ASM_OPERANDS))
+    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 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Segher Boessenkool]

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.

> +  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
> +      && (GET_CODE (PATTERN (i3)) == SET
> +	  && GET_CODE (SET_SRC (PATTERN (i3))) == ASM_OPERANDS))
> +    return 0;

That works only for asms with exactly one output.  You want
extract_asm_operands.


Segher

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

[Terry Guo]

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

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.
>
>> +  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
>> +      && (GET_CODE (PATTERN (i3)) == SET
>> +       && GET_CODE (SET_SRC (PATTERN (i3))) == ASM_OPERANDS))
>> +    return 0;
>
> That works only for asms with exactly one output.  You want
> extract_asm_operands.
>
>
> Segher

Thanks Segher. Patch is updated per you suggestion. Is this one ok for stage 1?

BR,
Terry

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

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.  */
+  if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)
+      && ((asm_op = extract_asm_operands (PATTERN (i3))) != NULL))
+    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 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Segher Boessenkool]

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

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

[Terry Guo]

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

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.

BR,
Terry

[-- 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 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

[Richard Sandiford]

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