| Message ID | 001e01d0394e$470bc390$d5234ab0$@arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 26, 2015 at 1:55 AM, Hale Wang <hale.wang@arm.com> wrote: > Hi, > > The GCC combine pass combines the insns even though they contain volatile > registers. This doesn't make sence. > > The test case listed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46164 > shows the expected asm command "mov r1, r1" is not generated."r1" is defined > as a volatile register, and there are three insns related to r1: > > (insn 98 97 40 3 (set (reg/v:SI 1 r1 [ b ]) (reg:SI 154 [ b ])) > (insn 41 40 43 3 (set (reg/f:SI 148) (reg/v:SI 1 r1 [ b ])) > (insn 43 41 45 3 (parallel [ > (set (reg/v:SI 0 r0 [ ret ]) > (asm_operands/v:SI ("mov %2, %2 mov %3, %3 mov %4, %4") > ("=r") 0 [ > (reg/v:SI 0 r0 [ a ]) > (reg/v:SI 1 r1 [ b ]) > (reg/v:SI 2 r2 [ c ]) > (mem/c:QI (reg/f:SI 148) [0 MEM[(char *)&temp]+0 S1 > A8]) > .... > The combine pass combine these insns: > > (note 98 97 40 3 NOTE_INSN_DELETED) > (note 41 40 43 3 NOTE_INSN_DELETED) > (insn 43 41 45 3 (parallel [ > (set (reg/v:SI 0 r0 [ ret ]) > (asm_operands/v:SI ("mov %2, %2 mov %3, %3 mov %4, %4") > ("=r") 0 [ > (reg/v:SI 0 r0 [ a ]) > (reg:SI 154 [ b ]) > (reg/v:SI 2 r2 [ c ]) > (mem/c:QI (reg:SI 154 [ b ]) [0 MEM[(char *)&temp]+0 > S1 A8]) > .... > > The volatile register "r1" is totally disappeared in the asm_operands, and > the generated asm code is unexpected. I think it is allowed to the second combining, just not the first. Also it is not about volatile registers here but rather user specified registers into inline-asm. Also I thought can_combine_p would reject combing into an inline-asm to prevent this issue. Thanks, Andrew > > This patch is used to disable the combine operation if the insns contain > volatile registers. A new test case is also added in this patch. > > Is it OK for trunk? > > BR, > Hale Wang > > ChangeLog: > > 2015-01-22 Hale Wang <hale.wang@arm.com> > > PR middle-end/46164 > * combine.c (can_combine_p): Don't combine the insns if > a volatile register is contained. > > 2015-01-22 Hale Wang <hale.wang@arm.com> > > PR middle-end/46164 > * gcc.target/arm/pr46164.c: New test. > > > diff --git a/gcc/combine.c b/gcc/combine.c > index 5c763b4..cf48666 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -2004,6 +2004,13 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn > *pred ATTRIBUTE_UNUSED, > return 0; > } > > + /* If src contains a volatile register, reject, because the register may > + possibly be used in a asm operand. The combined insn may cause the > asm > + operand to be generated unexpectly. */ > + > + if (REG_P (src) && REG_USERVAR_P (src)) > + return 0; > + > /* If INSN contains anything volatile, or is an `asm' (whether volatile > or not), reject, unless nothing volatile comes between it and I3 */ > > diff --git a/gcc/testsuite/gcc.target/arm/pr46164.c > b/gcc/testsuite/gcc.target/arm/pr46164.c > new file mode 100644 > index 0000000..ad3b7cb > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/pr46164.c > @@ -0,0 +1,26 @@ > +/* { dg-do compile } */ > +/* { dg-options "-mcpu=cortex-m3 -mthumb -O1" } */ > + > +char temp[16]; > +extern int foo1 (void); > + > +void foo (void) > +{ > + int i; > + int len; > + > + while (1) > + { > + len = foo1 (); > + register char *a asm ("r1") = temp; > + asm volatile ("mov %[r1], %[r1]\n " :: [r1]"r"(a), "m"(*a)); > + > + for (i = 0; i < len; i++) > + { > + if (temp[i] == 10) > + return; > + } > + } > +} > + > +/* { dg-final { scan-assembler "\[\\t \]+mov\ r1,\ r1" } } */
On Mon, Jan 26, 2015 at 05:55:52PM +0800, Hale Wang wrote: > The GCC combine pass combines the insns even though they contain volatile > registers. This doesn't make sence. > > The test case listed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46164 > shows the expected asm command "mov r1, r1" is not generated."r1" is defined > as a volatile register, and there are three insns related to r1: There is nothing volatile on reg/v, that is just a flag that the corresponding variable has been declared by the user. Preventing that to be combined would be a serious regression on code quality. Jakub
On Mon, Jan 26, 2015 at 05:55:52PM +0800, Hale Wang wrote: > The GCC combine pass combines the insns even though they contain volatile > registers. "Local register variable", "register asm". > This doesn't make sence. On the contrary, it makes a lot of sense: you *want* insns 41+43 to be combined in the below, that is the purpose of having local register variables at all! > The test case listed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46164 > shows the expected asm command "mov r1, r1" is not generated."r1" is defined > as a volatile register, and there are three insns related to r1: > > (insn 98 97 40 3 (set (reg/v:SI 1 r1 [ b ]) (reg:SI 154 [ b ])) > (insn 41 40 43 3 (set (reg/f:SI 148) (reg/v:SI 1 r1 [ b ])) > (insn 43 41 45 3 (parallel [ > (set (reg/v:SI 0 r0 [ ret ]) > (asm_operands/v:SI ("mov %2, %2 mov %3, %3 mov %4, %4") > ("=r") 0 [ > (reg/v:SI 0 r0 [ a ]) > (reg/v:SI 1 r1 [ b ]) > (reg/v:SI 2 r2 [ c ]) > (mem/c:QI (reg/f:SI 148) [0 MEM[(char *)&temp]+0 S1 > A8]) 98+41 was not combined. Why? 41+43 was not combined. Why? Answer that and you likely know how to solve this problem properly :-) Segher
> > The test case listed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46164 > > shows the expected asm command "mov r1, r1" is not generated."r1" is defined > > as a volatile register, and there are three insns related to r1: > > > > (insn 98 97 40 3 (set (reg/v:SI 1 r1 [ b ]) (reg:SI 154 [ b ])) > > (insn 41 40 43 3 (set (reg/f:SI 148) (reg/v:SI 1 r1 [ b ])) > > (insn 43 41 45 3 (parallel [ > > (set (reg/v:SI 0 r0 [ ret ]) > > (asm_operands/v:SI ("mov %2, %2 mov %3, %3 mov %4, %4") > > ("=r") 0 [ > > (reg/v:SI 0 r0 [ a ]) > > (reg/v:SI 1 r1 [ b ]) > > (reg/v:SI 2 r2 [ c ]) > > (mem/c:QI (reg/f:SI 148) [0 MEM[(char *)&temp]+0 S1 > > A8]) > > 98+41 was not combined. Why? 41+43 was not combined. Why? Actually, it looks like it first rejected 98+41, then combined 41+43 (and that is fine), and then it combined 98+43? Segher
diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..cf48666 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -2004,6 +2004,13 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED, return 0; } + /* If src contains a volatile register, reject, because the register may + possibly be used in a asm operand. The combined insn may cause the asm + operand to be generated unexpectly. */ + + if (REG_P (src) && REG_USERVAR_P (src)) + return 0; + /* If INSN contains anything volatile, or is an `asm' (whether volatile or not), reject, unless nothing volatile comes between it and I3 */ diff --git a/gcc/testsuite/gcc.target/arm/pr46164.c b/gcc/testsuite/gcc.target/arm/pr46164.c new file mode 100644 index 0000000..ad3b7cb --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr46164.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-mcpu=cortex-m3 -mthumb -O1" } */ + +char temp[16]; +extern int foo1 (void); + +void foo (void) +{ + int i; + int len; + + while (1) + { + len = foo1 (); + register char *a asm ("r1") = temp; + asm volatile ("mov %[r1], %[r1]\n " :: [r1]"r"(a), "m"(*a)); + + for (i = 0; i < len; i++) + { + if (temp[i] == 10) + return; + } + } +} + +/* { dg-final { scan-assembler "\[\\t \]+mov\ r1,\ r1" } } */
Hi, The GCC combine pass combines the insns even though they contain volatile registers. This doesn't make sence. The test case listed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46164 shows the expected asm command "mov r1, r1" is not generated."r1" is defined as a volatile register, and there are three insns related to r1: (insn 98 97 40 3 (set (reg/v:SI 1 r1 [ b ]) (reg:SI 154 [ b ])) (insn 41 40 43 3 (set (reg/f:SI 148) (reg/v:SI 1 r1 [ b ])) (insn 43 41 45 3 (parallel [ (set (reg/v:SI 0 r0 [ ret ]) (asm_operands/v:SI ("mov %2, %2 mov %3, %3 mov %4, %4") ("=r") 0 [ (reg/v:SI 0 r0 [ a ]) (reg/v:SI 1 r1 [ b ]) (reg/v:SI 2 r2 [ c ]) (mem/c:QI (reg/f:SI 148) [0 MEM[(char *)&temp]+0 S1 A8]) .... The combine pass combine these insns: (note 98 97 40 3 NOTE_INSN_DELETED) (note 41 40 43 3 NOTE_INSN_DELETED) (insn 43 41 45 3 (parallel [ (set (reg/v:SI 0 r0 [ ret ]) (asm_operands/v:SI ("mov %2, %2 mov %3, %3 mov %4, %4") ("=r") 0 [ (reg/v:SI 0 r0 [ a ]) (reg:SI 154 [ b ]) (reg/v:SI 2 r2 [ c ]) (mem/c:QI (reg:SI 154 [ b ]) [0 MEM[(char *)&temp]+0 S1 A8]) .... The volatile register "r1" is totally disappeared in the asm_operands, and the generated asm code is unexpected. This patch is used to disable the combine operation if the insns contain volatile registers. A new test case is also added in this patch. Is it OK for trunk? BR, Hale Wang ChangeLog: 2015-01-22 Hale Wang <hale.wang@arm.com> PR middle-end/46164 * combine.c (can_combine_p): Don't combine the insns if a volatile register is contained. 2015-01-22 Hale Wang <hale.wang@arm.com> PR middle-end/46164 * gcc.target/arm/pr46164.c: New test.