Support disabling inline caches for attribute access operations

Summary:
Making inline caches an optional optimization that's defaulted to on.

This diff adds back `LoadAttr`, `StoreAttr`, and `LoadMethod`, except they now
call out to runtime helpers instead of using inline caches.  There's a new
`JITRT_GetMethod` function for `LoadMethod`.

When inline caches are enabled, these instructions will simplify down to their
respective `*Cached` instructions. The simplifier prioritizes the original
simplifications to the inline cache versions, i.e. we'll try to simplify `LoadAttr` to
`LoadTypeAttrCacheItem` before we simplify down to `LoadAttrCached`.

Reviewed By: swtaarrs

Differential Revision: D55607248

fbshipit-source-id: e3e155d28c3b79f31f4097c924bb02d7cdc0ff66

cinderx/Jit/config.h

@@ -49,6 +49,8 @@ struct Config {
   // Assume that globals and builtins dictionaries, but not their contents, are
   // unchanged across Python function calls.
   bool stable_globals{true};
+  // Use inline caches for attribute accesses.
+  bool attr_caches{true};
   HIROptimizations hir_opts;
   size_t batch_compile_workers{0};
   // Sizes (in bytes) of the hot and cold code sections. Only applicable if

cinderx/Jit/hir/analysis.cpp

@@ -98,6 +98,7 @@ bool isPassthrough(const Instr& instr) {
     case Opcode::kListExtend:
     case Opcode::kLoadArg:
     case Opcode::kLoadArrayItem:
+    case Opcode::kLoadAttr:
     case Opcode::kLoadAttrCached:
     case Opcode::kLoadAttrSpecial:
     case Opcode::kLoadAttrSuper:
@@ -110,6 +111,7 @@ bool isPassthrough(const Instr& instr) {
     case Opcode::kLoadFunctionIndirect:
     case Opcode::kLoadGlobal:
     case Opcode::kLoadGlobalCached:
+    case Opcode::kLoadMethod:
     case Opcode::kLoadMethodCached:
     case Opcode::kLoadMethodSuper:
     case Opcode::kLoadModuleMethodCached:
@@ -146,6 +148,7 @@ bool isPassthrough(const Instr& instr) {
     case Opcode::kSetUpdate:
     case Opcode::kStealCellItem:
     case Opcode::kStoreArrayItem:
+    case Opcode::kStoreAttr:
     case Opcode::kStoreAttrCached:
     case Opcode::kStoreSubscr:
     case Opcode::kTpAlloc:

cinderx/Jit/hir/builder.cpp

@@ -2188,7 +2188,7 @@ void HIRBuilder::emitLoadAttr(
     const jit::BytecodeInstruction& bc_instr) {
   Register* receiver = tc.frame.stack.pop();
   Register* result = temps_.AllocateStack();
-  tc.emit<LoadAttrCached>(result, receiver, bc_instr.oparg(), tc.frame);
+  tc.emit<LoadAttr>(result, receiver, bc_instr.oparg(), tc.frame);
   tc.frame.stack.push(result);
 }
 
@@ -2199,7 +2199,7 @@ void HIRBuilder::emitLoadMethod(
   Register* receiver = tc.frame.stack.pop();
   Register* result = temps_.AllocateStack();
   Register* method_instance = temps_.AllocateStack();
-  tc.emit<LoadMethodCached>(result, receiver, bc_instr.oparg(), tc.frame);
+  tc.emit<LoadMethod>(result, receiver, bc_instr.oparg(), tc.frame);
   tc.emit<GetSecondOutput>(method_instance, TOptObject, result);
   tc.frame.stack.push(result);
   tc.frame.stack.push(method_instance);
@@ -3045,7 +3045,7 @@ void HIRBuilder::emitStoreAttr(
   Register* receiver = tc.frame.stack.pop();
   Register* value = tc.frame.stack.pop();
   Register* result = temps_.AllocateStack();
-  tc.emit<StoreAttrCached>(result, receiver, value, bc_instr.oparg(), tc.frame);
+  tc.emit<StoreAttr>(result, receiver, value, bc_instr.oparg(), tc.frame);
 }
 
 void HIRBuilder::moveOverwrittenStackRegisters(

cinderx/Jit/hir/hir.cpp

@@ -228,10 +228,12 @@ bool Instr::isReplayable() const {
     case Opcode::kIsTruthy:
     case Opcode::kListAppend:
     case Opcode::kListExtend:
+    case Opcode::kLoadAttr:
     case Opcode::kLoadAttrCached:
     case Opcode::kLoadAttrSpecial:
     case Opcode::kLoadAttrSuper:
     case Opcode::kLoadGlobal:
+    case Opcode::kLoadMethod:
     case Opcode::kLoadMethodCached:
     case Opcode::kLoadModuleMethodCached:
     case Opcode::kLoadMethodSuper:
@@ -261,6 +263,7 @@ bool Instr::isReplayable() const {
     case Opcode::kStoreField:
     case Opcode::kSnapshot:
     case Opcode::kStoreArrayItem:
+    case Opcode::kStoreAttr:
     case Opcode::kStoreAttrCached:
     case Opcode::kStoreSubscr:
     case Opcode::kTpAlloc:

cinderx/Jit/hir/hir.h

@@ -2437,17 +2437,33 @@ class DeoptBaseWithNameIdx : public DeoptBase {
   int name_idx_;
 };
 
-// Load an attribute from an object, using an inline cache.
+// Load an attribute from an object.
+DEFINE_SIMPLE_INSTR(
+    LoadAttr,
+    (TObject),
+    HasOutput,
+    Operands<1>,
+    DeoptBaseWithNameIdx);
+
+// Variant of LoadAttr that uses an inline cache.
 DEFINE_SIMPLE_INSTR(
     LoadAttrCached,
     (TObject),
     HasOutput,
     Operands<1>,
     DeoptBaseWithNameIdx);
 
-// Set the attribute of an object, using an inline cache.
+// Set the attribute of an object.
 //
 // Places NULL in dst if an error occurred or a non-NULL value otherwise
+DEFINE_SIMPLE_INSTR(
+    StoreAttr,
+    (TObject, TObject),
+    HasOutput,
+    Operands<2>,
+    DeoptBaseWithNameIdx);
+
+// Variant of StoreAttr that uses an inline cache.
 DEFINE_SIMPLE_INSTR(
     StoreAttrCached,
     (TObject, TObject),
@@ -2582,6 +2598,14 @@ class LoadMethodBase : public DeoptBaseWithNameIdx {
 
 // Like LoadAttr, but when we know that we're loading an attribute that will be
 // used for a method call.
+DEFINE_SIMPLE_INSTR(
+    LoadMethod,
+    (TObject),
+    HasOutput,
+    Operands<1>,
+    LoadMethodBase);
+
+// Variant of LoadMethod that uses an inline cache.
 DEFINE_SIMPLE_INSTR(
     LoadMethodCached,
     (TObject),

cinderx/Jit/hir/memory_effects.cpp

@@ -130,10 +130,12 @@ MemoryEffects memoryEffects(const Instr& inst) {
     case Opcode::kInvokeStaticFunction:
     case Opcode::kIsInstance:
     case Opcode::kIsTruthy:
+    case Opcode::kLoadAttr:
     case Opcode::kLoadAttrCached:
     case Opcode::kLoadAttrSpecial:
     case Opcode::kLoadAttrSuper:
     case Opcode::kLoadGlobal:
+    case Opcode::kLoadMethod:
     case Opcode::kLoadMethodCached:
     case Opcode::kLoadModuleMethodCached:
     case Opcode::kLoadMethodSuper:
@@ -162,6 +164,7 @@ MemoryEffects memoryEffects(const Instr& inst) {
     case Opcode::kSetDictItem:
     case Opcode::kSetSetItem:
     case Opcode::kSetUpdate:
+    case Opcode::kStoreAttr:
     case Opcode::kStoreAttrCached:
     case Opcode::kStoreSubscr:
       return {true, AEmpty, {}, AManagedHeapAny};

cinderx/Jit/hir/opcode.h

@@ -84,6 +84,7 @@ namespace jit::hir {
   V(LoadArrayItem)                     \
   V(LoadFieldAddress)                  \
   V(LoadArg)                           \
+  V(LoadAttr)                          \
   V(LoadAttrCached)                    \
   V(LoadAttrSpecial)                   \
   V(LoadAttrSuper)                     \
@@ -95,6 +96,7 @@ namespace jit::hir {
   V(LoadFunctionIndirect)              \
   V(LoadGlobalCached)                  \
   V(LoadGlobal)                        \
+  V(LoadMethod)                        \
   V(LoadMethodCached)                  \
   V(LoadModuleMethodCached)            \
   V(LoadMethodSuper)                   \
@@ -134,6 +136,7 @@ namespace jit::hir {
   V(Snapshot)                          \
   V(StealCellItem)                     \
   V(StoreArrayItem)                    \
+  V(StoreAttr)                         \
   V(StoreAttrCached)                   \
   V(StoreField)                        \
   V(StoreSubscr)                       \

cinderx/Jit/hir/parser.cpp

@@ -294,6 +294,12 @@ HIRParser::parseInstr(std::string_view opcode, Register* dst, int bb_index) {
     }
     expect(">");
     NEW_INSTR(LoadArg, dst, idx, ty);
+  } else if (opcode == "LoadMethod") {
+    expect("<");
+    int idx = GetNextNameIdx();
+    expect(">");
+    auto receiver = ParseRegister();
+    instruction = newInstr<LoadMethod>(dst, receiver, idx);
   } else if (opcode == "LoadMethodCached") {
     expect("<");
     int idx = GetNextNameIdx();
@@ -357,6 +363,12 @@ HIRParser::parseInstr(std::string_view opcode, Register* dst, int bb_index) {
   } else if (opcode == "Incref") {
     auto var = ParseRegister();
     NEW_INSTR(Incref, var);
+  } else if (opcode == "LoadAttr") {
+    expect("<");
+    int idx = GetNextNameIdx();
+    expect(">");
+    auto receiver = ParseRegister();
+    instruction = newInstr<LoadAttr>(dst, receiver, idx);
   } else if (opcode == "LoadAttrCached") {
     expect("<");
     int idx = GetNextNameIdx();
@@ -383,6 +395,13 @@ HIRParser::parseInstr(std::string_view opcode, Register* dst, int bb_index) {
         /*builtins=*/nullptr,
         /*globals=*/nullptr,
         name_idx);
+  } else if (opcode == "StoreAttr") {
+    expect("<");
+    int idx = GetNextNameIdx();
+    expect(">");
+    auto receiver = ParseRegister();
+    auto value = ParseRegister();
+    instruction = newInstr<StoreAttr>(dst, receiver, value, idx);
   } else if (opcode == "StoreAttrCached") {
     expect("<");
     int idx = GetNextNameIdx();

cinderx/Jit/hir/printer.cpp

@@ -486,6 +486,7 @@ static std::string format_immediates(const Instr& instr) {
       _Py_Identifier* id = load.id();
       return fmt::format("\"{}\"", id->string);
     }
+    case Opcode::kLoadMethod:
     case Opcode::kLoadMethodCached:
     case Opcode::kLoadModuleMethodCached: {
       const auto& load = static_cast<const LoadMethodBase&>(instr);
@@ -585,7 +586,9 @@ static std::string format_immediates(const Instr& instr) {
       return ss.str();
     }
     case Opcode::kDeleteAttr:
+    case Opcode::kLoadAttr:
     case Opcode::kLoadAttrCached:
+    case Opcode::kStoreAttr:
     case Opcode::kStoreAttrCached: {
       const auto& named = static_cast<const DeoptBaseWithNameIdx&>(instr);
       return format_name(named, named.name_idx());

cinderx/Jit/hir/simplify.cpp

@@ -499,16 +499,14 @@ Register* simplifyLoadVarObjectSize(Env& env, const LoadVarObjectSize* instr) {
 
 Register* simplifyLoadModuleMethodCached(
     Env& env,
-    const LoadMethodCached* load_meth) {
+    const LoadMethod* load_meth) {
   Register* receiver = load_meth->GetOperand(0);
   int name_idx = load_meth->name_idx();
   return env.emit<LoadModuleMethodCached>(
       receiver, name_idx, *load_meth->frameState());
 }
 
-Register* simplifyLoadTypeMethodCached(
-    Env& env,
-    const LoadMethodCached* load_meth) {
+Register* simplifyLoadTypeMethodCached(Env& env, const LoadMethod* load_meth) {
   Register* receiver = load_meth->GetOperand(0);
   const int cache_id = env.func.env.allocateLoadTypeMethodCache();
   env.emit<UseType>(receiver, TType);
@@ -529,7 +527,10 @@ Register* simplifyLoadTypeMethodCached(
       });
 }
 
-Register* simplifyLoadMethod(Env& env, const LoadMethodCached* load_meth) {
+Register* simplifyLoadMethod(Env& env, const LoadMethod* load_meth) {
+  if (!getConfig().attr_caches) {
+    return nullptr;
+  }
   Register* receiver = load_meth->GetOperand(0);
   Type ty = receiver->type();
   if (receiver->isA(TType)) {
@@ -539,7 +540,10 @@ Register* simplifyLoadMethod(Env& env, const LoadMethodCached* load_meth) {
   if (type == &PyModule_Type || type == &Ci_StrictModule_Type) {
     return simplifyLoadModuleMethodCached(env, load_meth);
   }
-  return nullptr;
+  return env.emit<LoadMethodCached>(
+      load_meth->GetOperand(0),
+      load_meth->name_idx(),
+      *load_meth->frameState());
 }
 
 Register* simplifyBinaryOp(Env& env, const BinaryOp* instr) {
@@ -798,7 +802,7 @@ Register* simplifyUnbox(Env& env, const Instr* instr) {
 // - The type doesn't have a descriptor at the attribute name.
 Register* simplifyLoadAttrSplitDict(
     Env& env,
-    const LoadAttrCached* load_attr,
+    const LoadAttr* load_attr,
     BorrowedRef<PyTypeObject> type,
     BorrowedRef<PyUnicodeObject> name) {
   if (!PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE) ||
@@ -985,7 +989,7 @@ Register* simplifyLoadAttrGenericDescriptor(Env& env, const DescrInfo& info) {
 // instances (not types).
 Register* simplifyLoadAttrInstanceReceiver(
     Env& env,
-    const LoadAttrCached* load_attr) {
+    const LoadAttr* load_attr) {
   Register* receiver = load_attr->GetOperand(0);
   Type type = receiver->type();
   BorrowedRef<PyTypeObject> py_type{type.runtimePyType()};
@@ -1026,9 +1030,7 @@ Register* simplifyLoadAttrInstanceReceiver(
   return nullptr;
 }
 
-Register* simplifyLoadAttrTypeReceiver(
-    Env& env,
-    const LoadAttrCached* load_attr) {
+Register* simplifyLoadAttrTypeReceiver(Env& env, const LoadAttr* load_attr) {
   Register* receiver = load_attr->GetOperand(0);
   if (!receiver->isA(TType)) {
     return nullptr;
@@ -1053,12 +1055,18 @@ Register* simplifyLoadAttrTypeReceiver(
       });
 }
 
-Register* simplifyLoadAttrCached(Env& env, const LoadAttrCached* load_attr) {
+Register* simplifyLoadAttr(Env& env, const LoadAttr* load_attr) {
   if (Register* reg = simplifyLoadAttrInstanceReceiver(env, load_attr)) {
     return reg;
   }
-  if (Register* reg = simplifyLoadAttrTypeReceiver(env, load_attr)) {
-    return reg;
+  if (getConfig().attr_caches) {
+    if (Register* reg = simplifyLoadAttrTypeReceiver(env, load_attr)) {
+      return reg;
+    }
+    return env.emit<LoadAttrCached>(
+        load_attr->GetOperand(0),
+        load_attr->name_idx(),
+        *load_attr->frameState());
   }
   return nullptr;
 }
@@ -1099,6 +1107,17 @@ Register* simplifyIsNegativeAndErrOccurred(
   return env.emit<LoadConst>(Type::fromCInt(0, output_type));
 }
 
+Register* simplifyStoreAttr(Env& env, const StoreAttr* store_attr) {
+  if (getConfig().attr_caches) {
+    return env.emit<StoreAttrCached>(
+        store_attr->GetOperand(0),
+        store_attr->GetOperand(1),
+        store_attr->name_idx(),
+        *store_attr->frameState());
+  }
+  return nullptr;
+}
+
 static bool isBuiltin(PyMethodDef* meth, const char* name) {
   // To make sure we have the right function, look up the PyMethodDef in the
   // fixed builtins. Any joker can make a new C method called "len", for
@@ -1321,12 +1340,10 @@ Register* simplifyInstr(Env& env, const Instr* instr) {
     case Opcode::kIsTruthy:
       return simplifyIsTruthy(env, static_cast<const IsTruthy*>(instr));
 
-    case Opcode::kLoadAttrCached:
-      return simplifyLoadAttrCached(
-          env, static_cast<const LoadAttrCached*>(instr));
-    case Opcode::kLoadMethodCached:
-      return simplifyLoadMethod(
-          env, static_cast<const LoadMethodCached*>(instr));
+    case Opcode::kLoadAttr:
+      return simplifyLoadAttr(env, static_cast<const LoadAttr*>(instr));
+    case Opcode::kLoadMethod:
+      return simplifyLoadMethod(env, static_cast<const LoadMethod*>(instr));
     case Opcode::kLoadField:
       return simplifyLoadField(env, static_cast<const LoadField*>(instr));
     case Opcode::kLoadTupleItem:
@@ -1360,6 +1377,9 @@ Register* simplifyInstr(Env& env, const Instr* instr) {
       return simplifyIsNegativeAndErrOccurred(
           env, static_cast<const IsNegativeAndErrOccurred*>(instr));
 
+    case Opcode::kStoreAttr:
+      return simplifyStoreAttr(env, static_cast<const StoreAttr*>(instr));
+
     case Opcode::kVectorCall:
       return simplifyVectorCall(env, static_cast<const VectorCall*>(instr));
     case Opcode::kVectorCallStatic:

cinderx/Jit/hir/ssa.cpp

@@ -392,10 +392,12 @@ Type outputType(
     case Opcode::kInPlaceOp:
     case Opcode::kInvokeIterNext:
     case Opcode::kInvokeMethod:
+    case Opcode::kLoadAttr:
     case Opcode::kLoadAttrCached:
     case Opcode::kLoadAttrSpecial:
     case Opcode::kLoadAttrSuper:
     case Opcode::kLoadGlobal:
+    case Opcode::kLoadMethod:
     case Opcode::kLoadMethodCached:
     case Opcode::kLoadModuleMethodCached:
     case Opcode::kLoadMethodSuper:
@@ -537,6 +539,7 @@ Type outputType(
     // respectively. At some point we should get rid of this extra layer and
     // deal with the int return value directly.
     case Opcode::kListExtend:
+    case Opcode::kStoreAttr:
     case Opcode::kStoreAttrCached:
       return TNoneType;