Fix property dependency generation for accelerated QML QObject properties

The previous approach of collecting the dependencies through an IR visitor
doesn't work, because it relies on a fixed structure - for example MEMBER(NAME,
prop) - which we can't guarantee (it's usually MEMBER(TEMP, prop)). But it
turns out that we can only pre-calculate dependencies for context, scope or id
properties, so we can do that right away in the QML specific JS codegen, store
that information in the IR function and use it from there in the data structure
generator as well as in the isel as a parameter to getQObjectProperty to tell
the run-time whether capture is required or not.

Change-Id: I33711c3420d6534c653c2a6a4284f0fc12e941cf
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

12 files changed, 48 insertions, 140 deletions

diff --git a/src/qml/compiler/qqmlcodegenerator.cpp b/src/qml/compiler/qqmlcodegenerator.cpp
index 19264493be..2b3cb80acb 100644
--- a/src/qml/compiler/qqmlcodegenerator.cpp
+++ b/src/qml/compiler/qqmlcodegenerator.cpp
@@ -1355,7 +1355,6 @@ V4IR::Expr *JSCodeGen::member(V4IR::Expr *base, const QString *name)
 
 V4IR::Expr *JSCodeGen::fallbackNameLookup(const QString &name, int line, int col)
 {
-    V4IR::Expr *result = 0;
     // Implement QML lookup semantics in the current file context.
     //
     // Note: We do not check if properties of the qml scope object or context object
@@ -1370,53 +1369,49 @@ V4IR::Expr *JSCodeGen::fallbackNameLookup(const QString &name, int line, int col
     // Look for IDs first.
     foreach (const IdMapping &mapping, _idObjects)
         if (name == mapping.name) {
-            result = _block->QML_CONTEXT_MEMBER(_block->NAME(V4IR::Name::builtin_qml_id_scope, line, col),
-                                                   _function->newString(mapping.name), mapping.idIndex);
-            break;
+            _function->idObjectDependencies.insert(mapping.idIndex);
+            return _block->QML_CONTEXT_MEMBER(_block->NAME(V4IR::Name::builtin_qml_id_scope, line, col),
+                                              _function->newString(mapping.name), mapping.idIndex);
         }
 
-    if (!result) {
+    {
         QQmlTypeNameCache::Result r = imports->query(name);
         if (r.isValid()) {
-            if (r.scriptIndex != -1) {
-                result = subscript(_block->NAME(V4IR::Name::builtin_qml_imported_scripts_object, line, col), _block->CONST(V4IR::NumberType, r.scriptIndex));
-            } else {
+            if (r.scriptIndex != -1)
+                return subscript(_block->NAME(V4IR::Name::builtin_qml_imported_scripts_object, line, col), _block->CONST(V4IR::NumberType, r.scriptIndex));
+            else
                 return 0; // TODO: We can't do fast lookup for these yet.
-            }
         }
     }
 
-    if (!result && _scopeObject) {
+    if (_scopeObject) {
         bool propertyExistsButForceNameLookup = false;
         QQmlPropertyData *pd = lookupQmlCompliantProperty(_scopeObject, name, &propertyExistsButForceNameLookup);
         if (propertyExistsButForceNameLookup)
             return 0;
         if (pd) {
+            if (!pd->isConstant())
+                _function->scopeObjectDependencies.insert(pd);
             int base = _block->newTemp();
             move(_block->TEMP(base), _block->NAME(V4IR::Name::builtin_qml_scope_object, line, col));
-            result = _block->QML_QOBJECT_PROPERTY(_block->TEMP(base),
-                                                  _function->newString(name), pd);
+            return  _block->QML_QOBJECT_PROPERTY(_block->TEMP(base), _function->newString(name), pd);
         }
     }
 
-    if (!result && _contextObject) {
+    if (_contextObject) {
         bool propertyExistsButForceNameLookup = false;
         QQmlPropertyData *pd = lookupQmlCompliantProperty(_contextObject, name, &propertyExistsButForceNameLookup);
         if (propertyExistsButForceNameLookup)
             return 0;
         if (pd) {
+            if (!pd->isConstant())
+                _function->contextObjectDependencies.insert(pd);
             int base = _block->newTemp();
             move(_block->TEMP(base), _block->NAME(V4IR::Name::builtin_qml_context_object, line, col));
-            result = _block->QML_QOBJECT_PROPERTY(_block->TEMP(base),
-                                                  _function->newString(name), pd);
+            return _block->QML_QOBJECT_PROPERTY(_block->TEMP(base), _function->newString(name), pd);
         }
     }
 
-    if (result) {
-        _function->hasQmlDependencies = true;
-        return result;
-    }
-
     // fall back to name lookup at run-time.
     return 0;
 }
diff --git a/src/qml/compiler/qv4compiler.cpp b/src/qml/compiler/qv4compiler.cpp
index b38c394bdc..cb17b86702 100644
--- a/src/qml/compiler/qv4compiler.cpp
+++ b/src/qml/compiler/qv4compiler.cpp
@@ -171,14 +171,10 @@ QV4::CompiledData::Unit *QV4::Compiler::JSUnitGenerator::generateUnit(int *total
         for (int i = 0; i < f->locals.size(); ++i)
             registerString(*f->locals.at(i));
 
-        if (f->hasQmlDependencies) {
-            QQmlJS::V4IR::QmlDependenciesCollector depCollector;
-
-            QSet<int> idObjectDeps;
-            QSet<QQmlPropertyData*> contextPropertyDeps;
-            QSet<QQmlPropertyData*> scopePropertyDeps;
-
-            depCollector.run(f, &idObjectDeps, &contextPropertyDeps, &scopePropertyDeps);
+        if (f->hasQmlDependencies()) {
+            QSet<int> idObjectDeps = f->idObjectDependencies;
+            QSet<QQmlPropertyData*> contextPropertyDeps = f->contextObjectDependencies;
+            QSet<QQmlPropertyData*> scopePropertyDeps = f->scopeObjectDependencies;
 
             if (!idObjectDeps.isEmpty())
                 qmlIdObjectDependenciesPerFunction.insert(f, idObjectDeps);
@@ -206,7 +202,7 @@ QV4::CompiledData::Unit *QV4::Compiler::JSUnitGenerator::generateUnit(int *total
         int qmlIdDepsCount = 0;
         int qmlPropertyDepsCount = 0;
 
-        if (f->hasQmlDependencies) {
+        if (f->hasQmlDependencies()) {
             IdDependencyHash::ConstIterator idIt = qmlIdObjectDependenciesPerFunction.find(f);
             if (idIt != qmlIdObjectDependenciesPerFunction.constEnd())
                 qmlIdDepsCount += idIt->count();
@@ -361,7 +357,7 @@ int QV4::Compiler::JSUnitGenerator::writeFunction(char *f, int index, QQmlJS::V4
     QSet<QQmlPropertyData*> qmlContextPropertyDeps;
     QSet<QQmlPropertyData*> qmlScopePropertyDeps;
 
-    if (irFunction->hasQmlDependencies) {
+    if (irFunction->hasQmlDependencies()) {
         qmlIdObjectDeps = qmlIdObjectDependenciesPerFunction.value(irFunction);
         qmlContextPropertyDeps = qmlContextPropertyDependenciesPerFunction.value(irFunction);
         qmlScopePropertyDeps = qmlScopePropertyDependenciesPerFunction.value(irFunction);
diff --git a/src/qml/compiler/qv4instr_moth_p.h b/src/qml/compiler/qv4instr_moth_p.h
index 9aaab2f105..9baf7f89ca 100644
--- a/src/qml/compiler/qv4instr_moth_p.h
+++ b/src/qml/compiler/qv4instr_moth_p.h
@@ -286,6 +286,7 @@ union Instr
         int propertyIndex;
         Param base;
         Param result;
+        bool captureRequired;
     };
     struct instr_storeProperty {
         MOTH_INSTR_HEADER
diff --git a/src/qml/compiler/qv4isel_masm.cpp b/src/qml/compiler/qv4isel_masm.cpp
index 95032eb13c..9d470c417d 100644
--- a/src/qml/compiler/qv4isel_masm.cpp
+++ b/src/qml/compiler/qv4isel_masm.cpp
@@ -559,7 +559,6 @@ JSC::MacroAssemblerCodeRef Assembler::link(int *codeSize)
 InstructionSelection::InstructionSelection(QV4::ExecutableAllocator *execAllocator, V4IR::Module *module, Compiler::JSUnitGenerator *jsGenerator)
     : EvalInstructionSelection(execAllocator, module, jsGenerator)
     , _block(0)
-    , _function(0)
     , _as(0)
 {
     compilationUnit = new CompilationUnit;
@@ -995,9 +994,10 @@ void InstructionSelection::getProperty(V4IR::Expr *base, const QString &name, V4
     }
 }
 
-void InstructionSelection::getQObjectProperty(V4IR::Expr *base, int propertyIndex, V4IR::Temp *target)
+void InstructionSelection::getQObjectProperty(V4IR::Expr *base, int propertyIndex, bool captureRequired, V4IR::Temp *target)
 {
-    generateFunctionCall(target, __qmljs_get_qobject_property, Assembler::ContextRegister, Assembler::PointerToValue(base), Assembler::TrustedImm32(propertyIndex));
+    generateFunctionCall(target, __qmljs_get_qobject_property, Assembler::ContextRegister, Assembler::PointerToValue(base), Assembler::TrustedImm32(propertyIndex),
+                         Assembler::TrustedImm32(captureRequired));
 }
 
 void InstructionSelection::setProperty(V4IR::Expr *source, V4IR::Expr *targetBase,
diff --git a/src/qml/compiler/qv4isel_masm_p.h b/src/qml/compiler/qv4isel_masm_p.h
index f84c981cd1..e3b41857ea 100644
--- a/src/qml/compiler/qv4isel_masm_p.h
+++ b/src/qml/compiler/qv4isel_masm_p.h
@@ -1496,7 +1496,7 @@ protected:
     virtual void getProperty(V4IR::Expr *base, const QString &name, V4IR::Temp *target);
     virtual void setProperty(V4IR::Expr *source, V4IR::Expr *targetBase, const QString &targetName);
     virtual void setQObjectProperty(V4IR::Expr *source, V4IR::Expr *targetBase, int propertyIndex);
-    virtual void getQObjectProperty(V4IR::Expr *base, int propertyIndex, V4IR::Temp *target);
+    virtual void getQObjectProperty(V4IR::Expr *base, int propertyIndex, bool captureRequired, V4IR::Temp *target);
     virtual void getElement(V4IR::Expr *base, V4IR::Expr *index, V4IR::Temp *target);
     virtual void setElement(V4IR::Expr *source, V4IR::Expr *targetBase, V4IR::Expr *targetIndex);
     virtual void copyValue(V4IR::Temp *sourceTemp, V4IR::Temp *targetTemp);
@@ -1651,7 +1651,6 @@ private:
     }
 
     V4IR::BasicBlock *_block;
-    V4IR::Function* _function;
     QSet<V4IR::Jump *> _removableJumps;
     Assembler* _as;
 
diff --git a/src/qml/compiler/qv4isel_moth.cpp b/src/qml/compiler/qv4isel_moth.cpp
index ccc0b5d98c..6db8f11a9e 100644
--- a/src/qml/compiler/qv4isel_moth.cpp
+++ b/src/qml/compiler/qv4isel_moth.cpp
@@ -154,7 +154,6 @@ inline bool isBoolType(V4IR::Expr *e)
 
 InstructionSelection::InstructionSelection(QV4::ExecutableAllocator *execAllocator, V4IR::Module *module, QV4::Compiler::JSUnitGenerator *jsGenerator)
     : EvalInstructionSelection(execAllocator, module, jsGenerator)
-    , _function(0)
     , _block(0)
     , _codeStart(0)
     , _codeNext(0)
@@ -509,12 +508,13 @@ void InstructionSelection::setQObjectProperty(V4IR::Expr *source, V4IR::Expr *ta
     addInstruction(store);
 }
 
-void InstructionSelection::getQObjectProperty(V4IR::Expr *base, int propertyIndex, V4IR::Temp *target)
+void InstructionSelection::getQObjectProperty(V4IR::Expr *base, int propertyIndex, bool captureRequired, V4IR::Temp *target)
 {
     Instruction::LoadQObjectProperty load;
     load.base = getParam(base);
     load.propertyIndex = propertyIndex;
     load.result = getResultParam(target);
+    load.captureRequired = captureRequired;
     addInstruction(load);
 }
 
diff --git a/src/qml/compiler/qv4isel_moth_p.h b/src/qml/compiler/qv4isel_moth_p.h
index 3a2fdb0c34..ffb8ff4539 100644
--- a/src/qml/compiler/qv4isel_moth_p.h
+++ b/src/qml/compiler/qv4isel_moth_p.h
@@ -125,7 +125,7 @@ protected:
     virtual void getProperty(V4IR::Expr *base, const QString &name, V4IR::Temp *target);
     virtual void setProperty(V4IR::Expr *source, V4IR::Expr *targetBase, const QString &targetName);
     virtual void setQObjectProperty(V4IR::Expr *source, V4IR::Expr *targetBase, int propertyIndex);
-    virtual void getQObjectProperty(V4IR::Expr *base, int propertyIndex, V4IR::Temp *target);
+    virtual void getQObjectProperty(V4IR::Expr *base, int propertyIndex, bool captureRequired, V4IR::Temp *target);
     virtual void getElement(V4IR::Expr *base, V4IR::Expr *index, V4IR::Temp *target);
     virtual void setElement(V4IR::Expr *source, V4IR::Expr *targetBase, V4IR::Expr *targetIndex);
     virtual void copyValue(V4IR::Temp *sourceTemp, V4IR::Temp *targetTemp);
@@ -168,7 +168,6 @@ private:
     void patchJumpAddresses();
     QByteArray squeezeCode() const;
 
-    V4IR::Function *_function;
     V4IR::BasicBlock *_block;
     V4IR::BasicBlock *_nextBlock;
 
diff --git a/src/qml/compiler/qv4isel_p.cpp b/src/qml/compiler/qv4isel_p.cpp
index 3f9e666c98..45b1e9f3b0 100644
--- a/src/qml/compiler/qv4isel_p.cpp
+++ b/src/qml/compiler/qv4isel_p.cpp
@@ -151,7 +151,12 @@ void IRDecoder::visitMove(V4IR::Move *s)
                     return;
                 }
             } else if (m->type == V4IR::Member::MemberOfQObject) {
-                getQObjectProperty(m->base, m->property->coreIndex, t);
+                bool captureRequired = true;
+                if (_function) {
+                    captureRequired = !_function->contextObjectDependencies.contains(m->property)
+                                      && !_function->scopeObjectDependencies.contains(m->property);
+                }
+                getQObjectProperty(m->base, m->property->coreIndex, captureRequired, t);
                 return;
             } else if (m->base->asTemp() || m->base->asConst()) {
                 getProperty(m->base, *m->name, t);
diff --git a/src/qml/compiler/qv4isel_p.h b/src/qml/compiler/qv4isel_p.h
index 5fba560d27..23ef7cc69e 100644
--- a/src/qml/compiler/qv4isel_p.h
+++ b/src/qml/compiler/qv4isel_p.h
@@ -100,6 +100,7 @@ namespace V4IR {
 class Q_QML_EXPORT IRDecoder: protected V4IR::StmtVisitor
 {
 public:
+    IRDecoder() : _function(0) {}
     virtual ~IRDecoder() = 0;
 
     virtual void visitPhi(V4IR::Phi *) {}
@@ -152,7 +153,7 @@ public: // to implement by subclasses:
     virtual void setActivationProperty(V4IR::Expr *source, const QString &targetName) = 0;
     virtual void initClosure(V4IR::Closure *closure, V4IR::Temp *target) = 0;
     virtual void getProperty(V4IR::Expr *base, const QString &name, V4IR::Temp *target) = 0;
-    virtual void getQObjectProperty(V4IR::Expr *base, int propertyIndex, V4IR::Temp *targetTemp) = 0;
+    virtual void getQObjectProperty(V4IR::Expr *base, int propertyIndex, bool captureRequired, V4IR::Temp *targetTemp) = 0;
     virtual void setProperty(V4IR::Expr *source, V4IR::Expr *targetBase, const QString &targetName) = 0;
     virtual void setQObjectProperty(V4IR::Expr *source, V4IR::Expr *targetBase, int propertyIndex) = 0;
     virtual void getElement(V4IR::Expr *base, V4IR::Expr *index, V4IR::Temp *target) = 0;
@@ -164,6 +165,8 @@ public: // to implement by subclasses:
 
 protected:
     virtual void callBuiltin(V4IR::Call *c, V4IR::Temp *result);
+
+    V4IR::Function *_function; // subclass needs to set
 };
 } // namespace IR
 
diff --git a/src/qml/compiler/qv4jsir.cpp b/src/qml/compiler/qv4jsir.cpp
index f8189b673d..75261b2469 100644
--- a/src/qml/compiler/qv4jsir.cpp
+++ b/src/qml/compiler/qv4jsir.cpp
@@ -1042,31 +1042,6 @@ void CloneExpr::visitMember(Member *e)
         Q_ASSERT(!"Unimplemented!");
 }
 
-void QmlDependenciesCollector::visitMember(Member *e) {
-    e->base->accept(this);
-    if (e->type == Member::MemberOfQmlContext) {
-        V4IR::Name *base = e->base->asName();
-        Q_ASSERT(base);
-        if (base->builtin == V4IR::Name::builtin_qml_id_scope)
-            _usedIdObjects.insert(e->memberIndex);
-    } else if (e->type == Member::MemberOfQObject
-             && !e->property->isFunction()) { // only non-functions have notifyIndex
-
-        if (Name *base = e->base->asName()) {
-            if (base->builtin == Name::builtin_qml_context_object)
-                _usedContextProperties.insert(e->property);
-            else if (base->builtin == Name::builtin_qml_scope_object)
-                _usedScopeProperties.insert(e->property);
-        }
-    }
-}
-
-void QmlDependenciesCollector::visitPhi(Phi *s) {
-    s->targetTemp->accept(this);
-    foreach (Expr *e, s->d->incoming)
-        e->accept(this);
-}
-
 } // end of namespace IR
 } // end of namespace QQmlJS
 
diff --git a/src/qml/compiler/qv4jsir_p.h b/src/qml/compiler/qv4jsir_p.h
index a7bed6419b..f4b8b15984 100644
--- a/src/qml/compiler/qv4jsir_p.h
+++ b/src/qml/compiler/qv4jsir_p.h
@@ -739,13 +739,19 @@ struct Function {
     uint isNamedExpression : 1;
     uint hasTry: 1;
     uint hasWith: 1;
-    uint hasQmlDependencies : 1;
-    uint unused : 24;
+    uint unused : 25;
 
     // Location of declaration in source code (-1 if not specified)
     int line;
     int column;
 
+    // Qml extension:
+    QSet<int> idObjectDependencies;
+    QSet<QQmlPropertyData*> contextObjectDependencies;
+    QSet<QQmlPropertyData*> scopeObjectDependencies;
+
+    bool hasQmlDependencies() const { return !idObjectDependencies.isEmpty() || !contextObjectDependencies.isEmpty() || !scopeObjectDependencies.isEmpty(); }
+
     template <typename _Tp> _Tp *New() { return new (pool->allocate(sizeof(_Tp))) _Tp(); }
 
     Function(Module *module, Function *outer, const QString &name)
@@ -761,7 +767,6 @@ struct Function {
         , isNamedExpression(false)
         , hasTry(false)
         , hasWith(false)
-        , hasQmlDependencies(false)
         , unused(0)
         , line(-1)
         , column(-1)
@@ -957,76 +962,6 @@ private:
     V4IR::Expr *cloned;
 };
 
-struct QmlDependenciesCollector : public V4IR::StmtVisitor, V4IR::ExprVisitor
-{
-    void run(Function *function, QSet<int> *idObjectDependencies, QSet<QQmlPropertyData*> *contextPropertyDependencies, QSet<QQmlPropertyData*> *scopePropertyDependencies)
-    {
-        QSet<int> idProperties;
-        QSet<QQmlPropertyData*> contextProperties;
-        QSet<QQmlPropertyData*> scopeProperties;
-        qSwap(_usedIdObjects, idProperties);
-        qSwap(_usedContextProperties, contextProperties);
-        qSwap(_usedScopeProperties, scopeProperties);
-        for (int i = 0; i < function->basicBlocks.count(); ++i) {
-            BasicBlock *bb = function->basicBlocks.at(i);
-            for (int j = 0; j < bb->statements.count(); ++j) {
-                Stmt *s = bb->statements.at(j);
-                s->accept(this);
-            }
-        }
-        qSwap(_usedScopeProperties, scopeProperties);
-        qSwap(_usedContextProperties, contextProperties);
-        qSwap(_usedIdObjects, idProperties);
-
-        *idObjectDependencies = idProperties;
-        *contextPropertyDependencies = contextProperties;
-        *scopePropertyDependencies = scopeProperties;
-    }
-
-protected:
-    QSet<int> _usedIdObjects;
-    QSet<QQmlPropertyData*> _usedContextProperties;
-    QSet<QQmlPropertyData*> _usedScopeProperties;
-
-    virtual void visitConst(Const *) {}
-    virtual void visitString(String *) {}
-    virtual void visitRegExp(RegExp *) {}
-    virtual void visitName(Name *) {}
-    virtual void visitTemp(Temp *) {}
-    virtual void visitClosure(Closure *) {}
-    virtual void visitConvert(Convert *e) { e->expr->accept(this); }
-    virtual void visitUnop(Unop *e) { e->expr->accept(this); }
-    virtual void visitBinop(Binop *e) { e->left->accept(this); e->right->accept(this); }
-
-    virtual void visitCall(Call *e) {
-        e->base->accept(this);
-        for (ExprList *it = e->args; it; it = it->next)
-            it->expr->accept(this);
-    }
-    virtual void visitNew(New *e) {
-        e->base->accept(this);
-        for (ExprList *it = e->args; it; it = it->next)
-            it->expr->accept(this);
-    }
-    virtual void visitSubscript(Subscript *e) {
-        e->base->accept(this);
-        e->index->accept(this);
-    }
-
-    virtual void visitMember(Member *e);
-
-    virtual void visitExp(Exp *s) {s->expr->accept(this);}
-    virtual void visitMove(Move *s) {
-        s->source->accept(this);
-        s->target->accept(this);
-    }
-
-    virtual void visitJump(Jump *) {}
-    virtual void visitCJump(CJump *s) { s->cond->accept(this); }
-    virtual void visitRet(Ret *s) { s->expr->accept(this); }
-    virtual void visitPhi(Phi *s);
-};
-
 } // end of namespace IR
 
 } // end of namespace QQmlJS
diff --git a/src/qml/compiler/qv4regalloc.cpp b/src/qml/compiler/qv4regalloc.cpp
index a531fd337d..a0bbbfd1df 100644
--- a/src/qml/compiler/qv4regalloc.cpp
+++ b/src/qml/compiler/qv4regalloc.cpp
@@ -427,7 +427,7 @@ protected: // IRDecoder
         addCall();
     }
 
-    virtual void getQObjectProperty(V4IR::Expr *base, int /*propertyIndex*/, V4IR::Temp *target)
+    virtual void getQObjectProperty(V4IR::Expr *base, int /*propertyIndex*/, bool /*captureRequired*/, V4IR::Temp *target)
     {
         addDef(target);
         addUses(base->asTemp(), Use::CouldHaveRegister);