Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On Thu, Feb 26, 2015 at 1:55 PM, Segher Boessenkool
seg...@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
seg...@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
pr64818-combine-user-specified-register.patch-5
Description: Binary data
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On Tue, Feb 17, 2015 at 11:39:34AM +0800, Terry Guo wrote:
On Sun, Feb 15, 2015 at 7:35 PM, Segher Boessenkool
seg...@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.
On Sun, Feb 15, 2015 at 7:35 PM, Segher Boessenkool seg...@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 pr64818-combine-user-specified-register.patch-4 Description: Binary data
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
On Fri, Feb 13, 2015 at 5:06 PM, Richard Sandiford rdsandif...@googlemail.com wrote: Segher Boessenkool seg...@kernel.crashing.org writes: On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: Hale Wang hale.w...@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 pr64818-combine-user-specified-register.patch-3 Description: Binary data
Re: Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
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.
Segher Boessenkool seg...@kernel.crashing.org writes: On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: Hale Wang hale.w...@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.
On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: Hale Wang hale.w...@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.
On 02/12/15 15:15, Segher Boessenkool wrote: On Thu, Feb 12, 2015 at 03:54:21PM +, Richard Sandiford wrote: Hale Wang hale.w...@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.
-Original Message- From: Segher Boessenkool [mailto:seg...@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 +, Richard Sandiford wrote: Hale Wang hale.w...@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.
Hale Wang hale.w...@arm.com writes: Ping? -Original Message- From: Hale Wang [mailto:hale.w...@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 seg...@kernel.crashing.org Hale Wang hale.w...@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 seg...@kernel.crashing.org Hale Wang hale.w...@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
Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.
Ping?
-Original Message-
From: Hale Wang [mailto:hale.w...@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 seg...@kernel.crashing.org
Hale Wang hale.w...@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 seg...@kernel.crashing.org
Hale Wang hale.w...@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 000..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
