| Message ID | 20110128021444.GA18059@intel.com |
|---|---|
| State | New |
| Headers | show |
Hi Eric, Is this patch OK for trunk? Thanks. H.J. --- On Thu, Jan 27, 2011 at 6:14 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: > It is a bad idea to combine asm statement. This patch disallows it. > Any comments? > > Thanks. > > > H.J. > --- > commit a11b95180e53d3a0bc3eabad5594859cea7c3334 > Author: H.J. Lu <hjl.tools@gmail.com> > Date: Thu Jan 27 18:12:27 2011 -0800 > > Never combine asm statement. > > diff --git a/gcc/ChangeLog.x32 b/gcc/ChangeLog.x32 > index 0ae4761..ef65e7b 100644 > --- a/gcc/ChangeLog.x32 > +++ b/gcc/ChangeLog.x32 > @@ -1,3 +1,8 @@ > +2011-01-27 H.J. Lu <hongjiu.lu@intel.com> > + > + PR rtl-optimization/47502 > + * combine.c (cant_combine_insn_p): Never combine asm statement. > + > 2011-01-25 H.J. Lu <hongjiu.lu@intel.com> > > PR target/47446 > diff --git a/gcc/combine.c b/gcc/combine.c > index 3ee53e6..d4a8079 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -2122,13 +2122,14 @@ cant_combine_insn_p (rtx insn) > /* Never combine loads and stores involving hard regs that are likely > to be spilled. The register allocator can usually handle such > reg-reg moves by tying. If we allow the combiner to make > - substitutions of likely-spilled regs, reload might die. > + substitutions of likely-spilled regs, reload might die. Never > + combine asm statement. > As an exception, we allow combinations involving fixed regs; these are > not available to the register allocator so there's no risk involved. */ > > set = single_set (insn); > if (! set) > - return 0; > + return asm_noperands (PATTERN (insn)) > 0; > src = SET_SRC (set); > dest = SET_DEST (set); > if (GET_CODE (src) == SUBREG) > @@ -2144,7 +2145,7 @@ cant_combine_insn_p (rtx insn) > && targetm.class_likely_spilled_p (REGNO_REG_CLASS (REGNO (dest)))))) > return 1; > > - return 0; > + return asm_noperands (src) > 0; > } > > struct likely_spilled_retval_info > diff --git a/gcc/testsuite/ChangeLog.x32 b/gcc/testsuite/ChangeLog.x32 > index 597294e..8759881 100644 > --- a/gcc/testsuite/ChangeLog.x32 > +++ b/gcc/testsuite/ChangeLog.x32 > @@ -1,3 +1,9 @@ > +2011-01-27 H.J. Lu <hongjiu.lu@intel.com> > + > + PR rtl-optimization/47502 > + * gcc.target/i386/pr47502-1.c: New. > + * gcc.target/i386/pr47502-2.c: Likewise. > + > 2011-01-25 H.J. Lu <hongjiu.lu@intel.com> > > PR target/47446 > diff --git a/gcc/testsuite/gcc.target/i386/pr47502-1.c b/gcc/testsuite/gcc.target/i386/pr47502-1.c > new file mode 100644 > index 0000000..727afe9 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/pr47502-1.c > @@ -0,0 +1,8 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O" } */ > + > +void > +foo (const void *xxxxx, void *yyyyy, long y) > +{ > + asm volatile ("" :: "c" ((xxxxx)), "d" ((yyyyy)), "S" (y)); > +} > diff --git a/gcc/testsuite/gcc.target/i386/pr47502-2.c b/gcc/testsuite/gcc.target/i386/pr47502-2.c > new file mode 100644 > index 0000000..1f57ea0 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/pr47502-2.c > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +int > +foo (int how, const void *set, void *oset) > +{ > + int resultvar; > + asm volatile ("" > + : "=a" (resultvar) > + : "0" (14) , "b" (how), "c" ((set)), "d" ((oset)), "S" (65 / 8) : "memory", "cc"); > + return resultvar; > +} >
On Mar 17, 2011, at 20:35, H.J. Lu wrote: >> - substitutions of likely-spilled regs, reload might die. >> + substitutions of likely-spilled regs, reload might die. Never >> + combine asm statement. > This has to be "statements", a plural. -Geert
On Thu, Mar 17, 2011 at 8:28 PM, Geert Bosch <bosch@adacore.com> wrote: > > On Mar 17, 2011, at 20:35, H.J. Lu wrote: > >>> - substitutions of likely-spilled regs, reload might die. >>> + substitutions of likely-spilled regs, reload might die. Never >>> + combine asm statement. >> > This has to be "statements", a plural. > I will make the change. Thanks.
> Is this patch OK for trunk?
This has always worked for the other ports AFAIK so I don't think we should
disable it without evaluating the impact on them. If reload has so many
problems with x32, maybe more fundamental changes should be made to the x86
back-end to support it.
On Fri, Mar 18, 2011 at 12:51 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Is this patch OK for trunk? > > This has always worked for the other ports AFAIK so I don't think we should > disable it without evaluating the impact on them. If reload has so many > problems with x32, maybe more fundamental changes should be made to the x86 > back-end to support it. > X32 port exposed many issues in GCC middle-end and RTL optimizations. The other ports never generate such asm statements combine has to deal with. Do you have any suggestions how to fix it in backend? Thanks.
On 03/18/2011 01:56 PM, H.J. Lu wrote: > On Fri, Mar 18, 2011 at 12:51 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>> Is this patch OK for trunk? >> >> This has always worked for the other ports AFAIK so I don't think we should >> disable it without evaluating the impact on them. If reload has so many >> problems with x32, maybe more fundamental changes should be made to the x86 >> back-end to support it. >> > > X32 port exposed many issues in GCC middle-end and RTL optimizations. > The other ports never generate such asm statements combine has to deal with. > Do you have any suggestions how to fix it in backend? How about analyzing the problem properly? All you posted was a patch, with no explanation of how the problem occurred. Without that, no one can say whether this is the x32 port really tickling a latent bug in the compiler, or whether it's a bug in your port. r~
On Fri, Mar 18, 2011 at 2:05 PM, Richard Henderson <rth@redhat.com> wrote: > On 03/18/2011 01:56 PM, H.J. Lu wrote: >> On Fri, Mar 18, 2011 at 12:51 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>>> Is this patch OK for trunk? >>> >>> This has always worked for the other ports AFAIK so I don't think we should >>> disable it without evaluating the impact on them. If reload has so many >>> problems with x32, maybe more fundamental changes should be made to the x86 >>> back-end to support it. >>> >> >> X32 port exposed many issues in GCC middle-end and RTL optimizations. >> The other ports never generate such asm statements combine has to deal with. >> Do you have any suggestions how to fix it in backend? > > How about analyzing the problem properly? > > All you posted was a patch, with no explanation of how the problem occurred. > Without that, no one can say whether this is the x32 port really tickling a > latent bug in the compiler, or whether it's a bug in your port. > See analysis in: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47502
On 03/18/2011 02:51 PM, H.J. Lu wrote: > See analysis in: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47502 You're patching the wrong place. See can_combine_p, which can test for specific sources, rather than cant_combine_insn_p which has no access to sources. r~
diff --git a/gcc/ChangeLog.x32 b/gcc/ChangeLog.x32 index 0ae4761..ef65e7b 100644 --- a/gcc/ChangeLog.x32 +++ b/gcc/ChangeLog.x32 @@ -1,3 +1,8 @@ +2011-01-27 H.J. Lu <hongjiu.lu@intel.com> + + PR rtl-optimization/47502 + * combine.c (cant_combine_insn_p): Never combine asm statement. + 2011-01-25 H.J. Lu <hongjiu.lu@intel.com> PR target/47446 diff --git a/gcc/combine.c b/gcc/combine.c index 3ee53e6..d4a8079 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -2122,13 +2122,14 @@ cant_combine_insn_p (rtx insn) /* Never combine loads and stores involving hard regs that are likely to be spilled. The register allocator can usually handle such reg-reg moves by tying. If we allow the combiner to make - substitutions of likely-spilled regs, reload might die. + substitutions of likely-spilled regs, reload might die. Never + combine asm statement. As an exception, we allow combinations involving fixed regs; these are not available to the register allocator so there's no risk involved. */ set = single_set (insn); if (! set) - return 0; + return asm_noperands (PATTERN (insn)) > 0; src = SET_SRC (set); dest = SET_DEST (set); if (GET_CODE (src) == SUBREG) @@ -2144,7 +2145,7 @@ cant_combine_insn_p (rtx insn) && targetm.class_likely_spilled_p (REGNO_REG_CLASS (REGNO (dest)))))) return 1; - return 0; + return asm_noperands (src) > 0; } struct likely_spilled_retval_info diff --git a/gcc/testsuite/ChangeLog.x32 b/gcc/testsuite/ChangeLog.x32 index 597294e..8759881 100644 --- a/gcc/testsuite/ChangeLog.x32 +++ b/gcc/testsuite/ChangeLog.x32 @@ -1,3 +1,9 @@ +2011-01-27 H.J. Lu <hongjiu.lu@intel.com> + + PR rtl-optimization/47502 + * gcc.target/i386/pr47502-1.c: New. + * gcc.target/i386/pr47502-2.c: Likewise. + 2011-01-25 H.J. Lu <hongjiu.lu@intel.com> PR target/47446 diff --git a/gcc/testsuite/gcc.target/i386/pr47502-1.c b/gcc/testsuite/gcc.target/i386/pr47502-1.c new file mode 100644 index 0000000..727afe9 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr47502-1.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +void +foo (const void *xxxxx, void *yyyyy, long y) +{ + asm volatile ("" :: "c" ((xxxxx)), "d" ((yyyyy)), "S" (y)); +} diff --git a/gcc/testsuite/gcc.target/i386/pr47502-2.c b/gcc/testsuite/gcc.target/i386/pr47502-2.c new file mode 100644 index 0000000..1f57ea0 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr47502-2.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +int +foo (int how, const void *set, void *oset) +{ + int resultvar; + asm volatile ("" + : "=a" (resultvar) + : "0" (14) , "b" (how), "c" ((set)), "d" ((oset)), "S" (65 / 8) : "memory", "cc"); + return resultvar; +}
It is a bad idea to combine asm statement. This patch disallows it. Any comments? Thanks. H.J. --- commit a11b95180e53d3a0bc3eabad5594859cea7c3334 Author: H.J. Lu <hjl.tools@gmail.com> Date: Thu Jan 27 18:12:27 2011 -0800 Never combine asm statement.