[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#792594: libqt5qml5 requires SSE2 on i386



Hi!

On Fri, 2015-10-09 at 21:09:32 -0300, Lisandro Damián Nicanor Pérez Meyer wrote:
> Hi Guillem! The patch really looks nice, thanks for it!

Thanks! I've reworked it a bit, to make it nicer, although I'm not
sure what are the Qt conventions all around so upstream might not
entirely like it.

Even though the cpu check logic is now abstracted, it kind of "leaks"
through the fatal error message, and upstream might want it protected
around a Q_PROCESSOR_X86_32 conditional perhaps, or that one
abstracted as well, dunno.

> But we really need to 
> upstream this and in order to achieve this we need you to either push it 
> trough Qt's gerrit instance and accept the CLA in the process (*please* read 
> below) or license the patch under a BSD-like license.

> CLA: the CLA is available in [cla]. **Please** note that, as stated in the 
> link, you still retain copyright over your contributions.
> 
> [cla] <http://www.qt.io/contributionagreement/>

I've read the CLA, and I don't think I can agree with point §3.1 which
states:

«… under license terms of The Qt Company’s choosing including any Open
Source Software license.»

Which to me implies an unfair advantage towards “The Qt Company” as
they might be able to choose a non-FLOSS license, but not other people
downloading the code. So given this I'd rather license the code as
something like MIT or BSD-3. Which would not remove the unfairness,
as the bulk of the code is LGPL and they can relicense the entire
thing but not other people, but at least I'm not participating in it.

> BSD: in case you don't want to agree with the CLA you can still put the patch 
> under a BSD-like license. In this case we are able to push the patch upstreams 
> ourselves, but if corrections are needed we need to act as proxies between you 
> and upstream's gerrit instance [cr], thus possibly adding noise to the system.

Sorry about that, but also having to register into another web site
seems like a bit of a drag. :)

> Finally the patch would need a longer description of it's intended purpose.

I've expanded on it a bit, let me know if it's not enough. I've also
retested it, and at least the plasmashell keeps starting and working,
like with the previous patch.

Thanks,
Guillem
Description: Do not make lack of SSE2 support on x86-32 fatal
 When an x86-32 CPU does not have SSE2 support (which is the case for
 all AMD CPUs, and older Intel CPUs), fallback to use the interpreter,
 otherwise use the JIT engine.
 .
 Even then, make the lack of SSE2 support on x86-32 fatal when trying
 to instantiate a JIT engine, which does require it.
 .
 Refactor the required CPU support check into a new privately exported
 function to avoid duplicating the logic, and do so in a function
 instead of a class member to avoid changing the class signatures.
Author: Guillem Jover <guillem@hadrons.org>
Origin: vendor
Bug-Debian: https://bugs.debian.org/792594
Last-Update: 2015-10-09

---
 src/qml/jit/qv4isel_masm.cpp    |    3 +++
 src/qml/jit/qv4isel_masm_p.h    |   10 ++++++++++
 src/qml/jsruntime/qv4engine.cpp |    4 ++--
 src/qml/qml/v8/qv8engine.cpp    |    7 -------
 tools/qmljs/qmljs.cpp           |    7 +++----
 5 files changed, 18 insertions(+), 13 deletions(-)

--- a/src/qml/qml/v8/qv8engine.cpp
+++ b/src/qml/qml/v8/qv8engine.cpp
@@ -58,7 +58,6 @@
 #include <QtCore/qjsonobject.h>
 #include <QtCore/qjsonvalue.h>
 #include <QtCore/qdatetime.h>
-#include <private/qsimd_p.h>
 
 #include <private/qv4value_inl_p.h>
 #include <private/qv4dateobject_p.h>
@@ -121,12 +120,6 @@ QV8Engine::QV8Engine(QJSEngine* qq)
     , m_xmlHttpRequestData(0)
     , m_listModelData(0)
 {
-#ifdef Q_PROCESSOR_X86_32
-    if (!qCpuHasFeature(SSE2)) {
-        qFatal("This program requires an X86 processor that supports SSE2 extension, at least a Pentium 4 or newer");
-    }
-#endif
-
     QML_MEMORY_SCOPE_STRING("QV8Engine::QV8Engine");
     qMetaTypeId<QJSValue>();
     qMetaTypeId<QList<int> >();
--- a/src/qml/jit/qv4isel_masm.cpp
+++ b/src/qml/jit/qv4isel_masm.cpp
@@ -198,6 +198,9 @@ InstructionSelection::InstructionSelecti
     , compilationUnit(new CompilationUnit)
     , qmlEngine(qmlEngine)
 {
+    if (!hasRequiredCpuSupport())
+        qFatal("This program requires an X86 processor that supports SSE2 extension, at least a Pentium 4 or newer");
+
     compilationUnit->codeRefs.resize(module->functions.size());
 }
 
--- a/src/qml/jsruntime/qv4engine.cpp
+++ b/src/qml/jsruntime/qv4engine.cpp
@@ -183,8 +183,8 @@ ExecutionEngine::ExecutionEngine(EvalISe
     if (!factory) {
 
 #ifdef V4_ENABLE_JIT
-        static const bool forceMoth = !qgetenv("QV4_FORCE_INTERPRETER").isEmpty();
-        if (forceMoth)
+        static const bool useMoth = !qgetenv("QV4_FORCE_INTERPRETER").isEmpty() || !JIT::hasRequiredCpuSupport();
+        if (useMoth)
             factory = new Moth::ISelFactory;
         else
             factory = new JIT::ISelFactory;
--- a/tools/qmljs/qmljs.cpp
+++ b/tools/qmljs/qmljs.cpp
@@ -140,11 +140,10 @@ int main(int argc, char *argv[])
     enum {
         use_masm,
         use_moth
-    } mode;
+    } mode = use_moth;
 #ifdef V4_ENABLE_JIT
-    mode = use_masm;
-#else
-    mode = use_moth;
+    if (QV4::JIT::hasRequiredCpuSupport())
+        mode = use_masm;
 #endif
 
     bool runAsQml = false;
--- a/src/qml/jit/qv4isel_masm_p.h
+++ b/src/qml/jit/qv4isel_masm_p.h
@@ -42,6 +42,7 @@
 
 #include <QtCore/QHash>
 #include <QtCore/QStack>
+#include <private/qsimd_p.h>
 #include <config.h>
 #include <wtf/Vector.h>
 
@@ -54,6 +55,15 @@ QT_BEGIN_NAMESPACE
 namespace QV4 {
 namespace JIT {
 
+inline bool Q_QML_PRIVATE_EXPORT hasRequiredCpuSupport()
+{
+#ifdef Q_PROCESSOR_X86_32
+    return qCpuHasFeature(SSE2);
+#else
+    return true;
+#endif
+}
+
 class Q_QML_EXPORT InstructionSelection:
         protected IR::IRDecoder,
         public EvalInstructionSelection

Reply to: