This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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
> >

Attachment: pr64818-combine-user-specified-register.changelog
Description: Binary data

Attachment: pr64818-combine-user-specified-register.patch-2
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]