Patchwork PATCH: PR rtl-optimization/47502: Never combine asm statement

login
register
mail settings
Submitter H.J. Lu
Date Jan. 28, 2011, 2:14 a.m.
Message ID <20110128021444.GA18059@intel.com>
Download mbox | patch
Permalink /patch/80761/
State New
Headers show

Comments

H.J. Lu - Jan. 28, 2011, 2:14 a.m.
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.
H.J. Lu - March 18, 2011, 12:35 a.m.
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;
> +}
>
Geert Bosch - March 18, 2011, 3:28 a.m.
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
H.J. Lu - March 18, 2011, 3:57 a.m.
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.
Eric Botcazou - March 18, 2011, 7:51 p.m.
> 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.
H.J. Lu - March 18, 2011, 8:56 p.m.
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.
Richard Henderson - March 18, 2011, 9:05 p.m.
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~
H.J. Lu - March 18, 2011, 9:51 p.m.
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
Richard Henderson - March 18, 2011, 10:39 p.m.
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~

Patch

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;
+}