Ping : [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained.

["Hale Wang" <hale dot wang at arm dot com>]

Ping?

> -----Original Message-----
> From: Hale Wang [mailto:hale.wang@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  <segher@kernel.crashing.org>
> 	    Hale Wang  <hale.wang@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  <segher@kernel.crashing.org>
> 	    Hale Wang  <hale.wang@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
> > >