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 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 > >
pr64818-combine-user-specified-register.changelog
Description: Binary data
pr64818-combine-user-specified-register.patch-2
Description: Binary data
