> -----Original Message----- > From: Segher Boessenkool [mailto:seg...@kernel.crashing.org] > Sent: Tuesday, January 27, 2015 12:52 PM > To: Hale Wang > Cc: GCC Patches > Subject: Re: [PATCH] [gcc, combine] PR46164: Don't combine the insns if a > volatile register is contained. > > On Tue, Jan 27, 2015 at 11:49:55AM +0800, Hale Wang wrote: > > Hi Hale, > > > > > 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" } */ > > > > > > Just "-O1" reproduces the problem here, FWIW. > > > > 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 > > > diff --git a/gcc/combine.c b/gcc/combine.c index 58de157..9cba594 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -1928,6 +1928,10 @@ 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 eliminate a register variable. */ 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
