ICs: don't emit guards for callattr and runtimeCall ICs when the called function excepts all args

Author: undingen

src/runtime/objmodel.cpp

@@ -3674,31 +3674,6 @@ Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope scope, CallattrRe
 
     int npassed_args = argspec.totalPassed();
 
-    if (rewrite_args && !rewrite_args->args_guarded) {
-        // TODO duplication with runtime_call
-        // TODO should know which args don't need to be guarded, ex if we're guaranteed that they
-        // already fit, either since the type inferencer could determine that,
-        // or because they only need to fit into an UNKNOWN slot.
-
-        if (npassed_args >= 1)
-            rewrite_args->arg1->addAttrGuard(offsetof(Box, cls), (intptr_t)arg1->cls);
-        if (npassed_args >= 2)
-            rewrite_args->arg2->addAttrGuard(offsetof(Box, cls), (intptr_t)arg2->cls);
-        if (npassed_args >= 3)
-            rewrite_args->arg3->addAttrGuard(offsetof(Box, cls), (intptr_t)arg3->cls);
-
-        if (npassed_args > 3) {
-            for (int i = 3; i < npassed_args; i++) {
-                // TODO if there are a lot of args (>16), might be better to increment a pointer
-                // rather index them directly?
-                RewriterVar* v = rewrite_args->args->getAttr((i - 3) * sizeof(Box*), Location::any());
-                v->addAttrGuard(offsetof(Box, cls), (intptr_t)args[i - 3]->cls);
-            }
-        }
-
-        rewrite_args->args_guarded = true;
-    }
-
     // right now I don't think this is ever called with INST_ONLY?
     assert(scope != INST_ONLY);
 
@@ -5083,6 +5058,36 @@ const char* PyEval_GetFuncDesc(PyObject* func) noexcept {
     }
 }
 
+static void addArgGuards(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Box* arg1, Box* arg2, Box* arg3,
+                         Box** args) {
+    // TODO should know which args don't need to be guarded, ex if we're guaranteed that they
+    // already fit, either since the type inferencer could determine that,
+    // or because they only need to fit into an UNKNOWN slot.
+
+    int kwargs_index = -1;
+    if (argspec.has_kwargs)
+        kwargs_index = argspec.kwargsIndex();
+
+    for (int i = 0; i < argspec.totalPassed(); i++) {
+        Box* v = getArg(i, arg1, arg2, arg3, args);
+
+        if (i == kwargs_index) {
+            if (v == NULL) {
+                // I don't think this case should ever get hit currently -- the only places
+                // we offer rewriting are places that don't have the ability to pass a NULL
+                // kwargs.
+                getArg(i, rewrite_args)->addGuard(0);
+            } else {
+                getArg(i, rewrite_args)->addAttrGuard(offsetof(Box, cls), (intptr_t)v->cls);
+            }
+        } else {
+            assert(v);
+            getArg(i, rewrite_args)->addAttrGuard(offsetof(Box, cls), (intptr_t)v->cls);
+        }
+    }
+    rewrite_args->args_guarded = true;
+}
+
 
 template <ExceptionStyle S, Rewritable rewritable>
 Box* runtimeCallInternal(Box* obj, CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Box* arg1, Box* arg2, Box* arg3,
@@ -5173,65 +5178,46 @@ Box* runtimeCallInternal(Box* obj, CallRewriteArgs* rewrite_args, ArgPassSpec ar
         return rtn;
     }
 
-    if (rewrite_args) {
-        if (!rewrite_args->args_guarded) {
-            // TODO should know which args don't need to be guarded, ex if we're guaranteed that they
-            // already fit, either since the type inferencer could determine that,
-            // or because they only need to fit into an UNKNOWN slot.
-
-            int kwargs_index = -1;
-            if (argspec.has_kwargs)
-                kwargs_index = argspec.kwargsIndex();
-
-            for (int i = 0; i < npassed_args; i++) {
-                Box* v = getArg(i, arg1, arg2, arg3, args);
-
-                if (i == kwargs_index) {
-                    if (v == NULL) {
-                        // I don't think this case should ever get hit currently -- the only places
-                        // we offer rewriting are places that don't have the ability to pass a NULL
-                        // kwargs.
-                        getArg(i, rewrite_args)->addGuard(0);
-                    } else {
-                        getArg(i, rewrite_args)->addAttrGuard(offsetof(Box, cls), (intptr_t)v->cls);
-                    }
-                } else {
-                    assert(v);
-                    getArg(i, rewrite_args)->addAttrGuard(offsetof(Box, cls), (intptr_t)v->cls);
-                }
-            }
-            rewrite_args->args_guarded = true;
-        }
-    }
 
     if (obj->cls == function_cls || obj->cls == builtin_function_or_method_cls) {
         BoxedFunctionBase* f = static_cast<BoxedFunctionBase*>(obj);
-
-        if (rewrite_args && !rewrite_args->func_guarded) {
-            rewrite_args->obj->addGuard((intptr_t)f);
-            rewrite_args->func_guarded = true;
-            rewrite_args->rewriter->addDependenceOn(f->dependent_ics);
-        }
+        auto md = f->md;
 
         // Some functions are sufficiently important that we want them to be able to patchpoint themselves;
         // they can do this by setting the "internal_callable" field:
-        auto callable = f->md->internal_callable.get<S>();
-
+        auto callable = md->internal_callable.get<S>();
         if (S == CAPI)
-            assert((bool(f->md->internal_callable.get(CXX)) == bool(callable))
+            assert((bool(md->internal_callable.get(CXX)) == bool(callable))
                    && "too many opportunities for mistakes unless both CXX and CAPI versions are implemented");
         else
-            assert((bool(f->md->internal_callable.get(CAPI)) == bool(callable))
+            assert((bool(md->internal_callable.get(CAPI)) == bool(callable))
                    && "too many opportunities for mistake unless both CXX and CAPI versions are implementeds");
 
         if (callable == NULL) {
             callable = callFunc<S>;
         }
 
+        if (rewrite_args && !rewrite_args->args_guarded) {
+            bool does_not_need_guards = callable == &callFunc<S> && md->always_use_version;
+            if (!does_not_need_guards)
+                addArgGuards(rewrite_args, argspec, arg1, arg2, arg3, args);
+        }
+
+        if (rewrite_args && !rewrite_args->func_guarded) {
+            rewrite_args->obj->addGuard((intptr_t)f);
+            rewrite_args->func_guarded = true;
+            // callFunc will add the invalidator
+            if (callable != &callFunc<S>)
+                rewrite_args->rewriter->addDependenceOn(f->dependent_ics);
+        }
+
         KEEP_ALIVE(f);
         Box* res = callable(f, rewrite_args, argspec, arg1, arg2, arg3, args, keyword_names);
         return res;
     } else if (obj->cls == instancemethod_cls) {
+        if (rewrite_args && !rewrite_args->args_guarded)
+            addArgGuards(rewrite_args, argspec, arg1, arg2, arg3, args);
+
         BoxedInstanceMethod* im = static_cast<BoxedInstanceMethod*>(obj);
 
         RewriterVar* r_im_func = NULL;

test/tests/func_ics.py

@@ -0,0 +1,10 @@
+# statcheck: stats['ic_attempts'] <= 2000
+# statcheck: stats['ic_attempts_skipped_megamorphic'] == 0
+ins = (1, 1.0, 1l, "", u"")
+def f():
+    for x in ins:
+        isinstance(x, int)
+        [].append(x)
+
+for i in xrange(30000):
+    f()