Don't combine instructions if they can't be copied

Don't combine instructions if they can't be copied

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Compiling the test case in the attached patch using arm-linux-gnueabi 
target g++ with the options "-O2 -fweb -fPIC -fvisibility=hidden" will 
give an error:

Assembler messages:
Error: symbol `.LPIC0' is already defined

That's because an instruction, which can't be copied, is combined into 
another instruction while that non-copyable instruction is still kept 
since its result is needed later. GCC will generate the same label 
".LPIC0" for that non-copyable instruction and the combined instruction 
since the label number is also copied and same for both instructions.

With this patch, GCC will not combine instructions if they can't be 
copied. Tested on arm-linux-gnueabi with c,c++,lto. No regressions are 
found.

Is it OK?


Regards,
-- 
Jie Zhang
CodeSourcery

[-- Attachment #2: gcc-combine-cannot-copy-insn-p.diff --]
[-- Type: text/x-patch, Size: 2938 bytes --]


	* combine.c (try_combine): Don't combine if can't copy
	instructions.

	testsuite/
	g++.dg/opt/combine.c: New test.

Index: testsuite/g++.dg/opt/combine.C
===================================================================
--- testsuite/g++.dg/opt/combine.C	(revision 0)
+++ testsuite/g++.dg/opt/combine.C	(revision 0)
@@ -0,0 +1,72 @@
+// { dg-do assemble { target fpic } }
+// { dg-options "-O2 -fweb -fPIC -fvisibility=hidden" }
+
+class QBasicAtomicInt
+{
+public:
+  volatile int _q_value;
+  inline operator int () const {return _q_value;}
+};
+class QVariant;
+class QScriptContext;
+class QScriptEngine;
+class QScriptValue
+{
+public:
+  QVariant toVariant () const;
+};
+class QScriptDebuggerBackendPrivate
+{
+  static QScriptValue trace (QScriptContext *context);
+};
+template <typename T> struct QMetaTypeId { };
+template <typename T> struct QMetaTypeId2
+{
+  static inline int qt_metatype_id ()
+  {
+    return QMetaTypeId<T>::qt_metatype_id () ;
+  }
+};
+template <typename T> inline int qMetaTypeId (T * = 0)
+{
+  return QMetaTypeId2<T>::qt_metatype_id () ;
+}
+class QVariant { };
+template<typename T> inline T qvariant_cast (const QVariant &v)
+{
+  const int vid = qMetaTypeId<T> ((0)) ;
+};
+class QScriptContext
+{
+public: 
+  QScriptValue callee () const;
+};
+class QScriptEngine  
+{
+public:
+  static bool convertV2 (const QScriptValue &value , int type , void *ptr) ;
+};
+inline bool qscriptvalue_cast_helper (const QScriptValue &value , int type , void *ptr)
+{
+  return QScriptEngine::convertV2 (value, type, ptr) ;
+}
+template<typename T> T qscriptvalue_cast (const QScriptValue &value)
+{
+  T t;
+  const int id = qMetaTypeId<T> () ;
+  if ( qscriptvalue_cast_helper (value, id, &t))
+    return qvariant_cast<T> (value.toVariant ()) ;
+}
+template <> struct QMetaTypeId< QScriptDebuggerBackendPrivate* >
+{
+  static int qt_metatype_id ()
+  {
+    static QBasicAtomicInt metatype_id = { (0) };
+    return metatype_id;
+  }
+};
+QScriptValue QScriptDebuggerBackendPrivate::trace (QScriptContext *context)
+{
+  QScriptValue data = context->callee () ;
+  QScriptDebuggerBackendPrivate *self = qscriptvalue_cast<QScriptDebuggerBackendPrivate*> (data) ;
+}
Index: combine.c
===================================================================
--- combine.c	(revision 165798)
+++ combine.c	(working copy)
@@ -2917,6 +2917,15 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
   else
     added_sets_0 = 0;
 
+  if (targetm.cannot_copy_insn_p
+      && ((added_sets_2 && targetm.cannot_copy_insn_p (i2))
+	  || (i1 && added_sets_1 && targetm.cannot_copy_insn_p (i1))
+	  || (i0 && added_sets_0 && targetm.cannot_copy_insn_p (i0))))
+    {
+      undo_all ();
+      return 0;
+    }
+
   /* If the set in I2 needs to be kept around, we must make a copy of
      PATTERN (I2), so that when we substitute I1SRC for I1DEST in
      PATTERN (I2), we are only substituting for the original I1DEST, not into

Re: Don't combine instructions if they can't be copied

[Eric Botcazou]

> With this patch, GCC will not combine instructions if they can't be
> copied. Tested on arm-linux-gnueabi with c,c++,lto. No regressions are
> found.
>
> Is it OK?

From the above description, as well as the ChangeLog, I would have said no, 
this is too broad.  But the implementation is actually more fine-grained and 
looks sensible to me.  So OK for mainline if (1) you add a comment to the code 
explaining its purpose, e.g. "We are about to copy insns for the case where 
they need to be kept around.  Check that..." and (2) you fix the ChangeLog.

No need to resubmit.

-- 
Eric Botcazou

Re: Don't combine instructions if they can't be copied

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On 10/23/2010 10:56 PM, Eric Botcazou wrote:
>> With this patch, GCC will not combine instructions if they can't be
>> copied. Tested on arm-linux-gnueabi with c,c++,lto. No regressions are
>> found.
>>
>> Is it OK?
>
>  From the above description, as well as the ChangeLog, I would have said no,
> this is too broad.  But the implementation is actually more fine-grained and
> looks sensible to me.  So OK for mainline if (1) you add a comment to the code
> explaining its purpose, e.g. "We are about to copy insns for the case where
> they need to be kept around.  Check that..." and (2) you fix the ChangeLog.
>
> No need to resubmit.
>
Thanks for review. I'm going to commit the attached patch soon with the 
above two points addressed.


Regards,
-- 
Jie Zhang
CodeSourcery

[-- Attachment #2: gcc-combine-cannot-copy-insn-p-2.diff --]
[-- Type: text/x-patch, Size: 3137 bytes --]


	* combine.c (try_combine): If insns need to be kept around,
	check that they can be copied in the merged instruction.

	testsuite/
	g++.dg/opt/combine.c: New test.

Index: testsuite/g++.dg/opt/combine.C
===================================================================
--- testsuite/g++.dg/opt/combine.C	(revision 0)
+++ testsuite/g++.dg/opt/combine.C	(revision 0)
@@ -0,0 +1,72 @@
+// { dg-do assemble { target fpic } }
+// { dg-options "-O2 -fweb -fPIC -fvisibility=hidden" }
+
+class QBasicAtomicInt
+{
+public:
+  volatile int _q_value;
+  inline operator int () const {return _q_value;}
+};
+class QVariant;
+class QScriptContext;
+class QScriptEngine;
+class QScriptValue
+{
+public:
+  QVariant toVariant () const;
+};
+class QScriptDebuggerBackendPrivate
+{
+  static QScriptValue trace (QScriptContext *context);
+};
+template <typename T> struct QMetaTypeId { };
+template <typename T> struct QMetaTypeId2
+{
+  static inline int qt_metatype_id ()
+  {
+    return QMetaTypeId<T>::qt_metatype_id () ;
+  }
+};
+template <typename T> inline int qMetaTypeId (T * = 0)
+{
+  return QMetaTypeId2<T>::qt_metatype_id () ;
+}
+class QVariant { };
+template<typename T> inline T qvariant_cast (const QVariant &v)
+{
+  const int vid = qMetaTypeId<T> ((0)) ;
+};
+class QScriptContext
+{
+public: 
+  QScriptValue callee () const;
+};
+class QScriptEngine  
+{
+public:
+  static bool convertV2 (const QScriptValue &value , int type , void *ptr) ;
+};
+inline bool qscriptvalue_cast_helper (const QScriptValue &value , int type , void *ptr)
+{
+  return QScriptEngine::convertV2 (value, type, ptr) ;
+}
+template<typename T> T qscriptvalue_cast (const QScriptValue &value)
+{
+  T t;
+  const int id = qMetaTypeId<T> () ;
+  if ( qscriptvalue_cast_helper (value, id, &t))
+    return qvariant_cast<T> (value.toVariant ()) ;
+}
+template <> struct QMetaTypeId< QScriptDebuggerBackendPrivate* >
+{
+  static int qt_metatype_id ()
+  {
+    static QBasicAtomicInt metatype_id = { (0) };
+    return metatype_id;
+  }
+};
+QScriptValue QScriptDebuggerBackendPrivate::trace (QScriptContext *context)
+{
+  QScriptValue data = context->callee () ;
+  QScriptDebuggerBackendPrivate *self = qscriptvalue_cast<QScriptDebuggerBackendPrivate*> (data) ;
+}
Index: combine.c
===================================================================
--- combine.c	(revision 165910)
+++ combine.c	(working copy)
@@ -2917,6 +2917,18 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
   else
     added_sets_0 = 0;
 
+  /* We are about to copy insns for the case where they need to be kept
+     around.  Check that they can be copied in the merged instruction.  */
+
+  if (targetm.cannot_copy_insn_p
+      && ((added_sets_2 && targetm.cannot_copy_insn_p (i2))
+	  || (i1 && added_sets_1 && targetm.cannot_copy_insn_p (i1))
+	  || (i0 && added_sets_0 && targetm.cannot_copy_insn_p (i0))))
+    {
+      undo_all ();
+      return 0;
+    }
+
   /* If the set in I2 needs to be kept around, we must make a copy of
      PATTERN (I2), so that when we substitute I1SRC for I1DEST in
      PATTERN (I2), we are only substituting for the original I1DEST, not into

Re: Don't combine instructions if they can't be copied

[Jie Zhang]

On 10/25/2010 02:09 PM, Jie Zhang wrote:
> 	testsuite/
> 	g++.dg/opt/combine.c: New test.

I just found a leading "*" was missing in this entry. I have committed a 
fix for it.


Regards,
-- 
Jie Zhang
CodeSourcery