* Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
@ 2015-04-21 1:39 Terry Guo
2015-04-21 3:03 ` Segher Boessenkool
0 siblings, 1 reply; 7+ messages in thread
From: Terry Guo @ 2015-04-21 1:39 UTC (permalink / raw)
To: Segher Boessenkool, law
Cc: Richard Sandiford, Richard Sandiford, Hale Wang, GCC Patches
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
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
2015-04-21 1:39 Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained Terry Guo
@ 2015-04-21 3:03 ` Segher Boessenkool
2015-04-21 7:13 ` Terry Guo
0 siblings, 1 reply; 7+ messages in thread
From: Segher Boessenkool @ 2015-04-21 3:03 UTC (permalink / raw)
To: Terry Guo
Cc: law, Richard Sandiford, Richard Sandiford, Hale Wang, GCC Patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
2015-04-21 3:03 ` Segher Boessenkool
@ 2015-04-21 7:13 ` Terry Guo
2015-04-22 1:44 ` Segher Boessenkool
0 siblings, 1 reply; 7+ messages in thread
From: Terry Guo @ 2015-04-21 7:13 UTC (permalink / raw)
To: Segher Boessenkool
Cc: law, Richard Sandiford, Richard Sandiford, Hale Wang, GCC Patches
[-- 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" } } */
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
2015-04-21 7:13 ` Terry Guo
@ 2015-04-22 1:44 ` Segher Boessenkool
2015-04-22 2:21 ` Terry Guo
0 siblings, 1 reply; 7+ messages in thread
From: Segher Boessenkool @ 2015-04-22 1:44 UTC (permalink / raw)
To: Terry Guo
Cc: law, Richard Sandiford, Richard Sandiford, Hale Wang, GCC Patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
2015-04-22 1:44 ` Segher Boessenkool
@ 2015-04-22 2:21 ` Terry Guo
2015-04-22 2:30 ` Segher Boessenkool
0 siblings, 1 reply; 7+ messages in thread
From: Terry Guo @ 2015-04-22 2:21 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: law, Hale Wang, GCC Patches, Richard Sandiford
[-- 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" } } */
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
2015-04-22 2:21 ` Terry Guo
@ 2015-04-22 2:30 ` Segher Boessenkool
2015-04-22 7:23 ` Terry Guo
0 siblings, 1 reply; 7+ messages in thread
From: Segher Boessenkool @ 2015-04-22 2:30 UTC (permalink / raw)
To: Terry Guo; +Cc: law, Hale Wang, GCC Patches, Richard Sandiford
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
2015-04-22 2:30 ` Segher Boessenkool
@ 2015-04-22 7:23 ` Terry Guo
0 siblings, 0 replies; 7+ messages in thread
From: Terry Guo @ 2015-04-22 7:23 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: law, Hale Wang, GCC Patches, Richard Sandiford
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-22 7:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-21 1:39 Ping^3 : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained Terry Guo
2015-04-21 3:03 ` Segher Boessenkool
2015-04-21 7:13 ` Terry Guo
2015-04-22 1:44 ` Segher Boessenkool
2015-04-22 2:21 ` Terry Guo
2015-04-22 2:30 ` Segher Boessenkool
2015-04-22 7:23 ` Terry Guo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).