Skip to content

Commit 90220f6

Browse files
yoneymeta-codesync[bot]
authored andcommitted
Add FT-safe list subscript HIR
Summary: Adds `ListSubscr`, an FT-safe specialization for exact-list subscripts that cannot use the borrowed `LoadArrayItem` path. It lowers to a runtime helper using `PyList_GetItemRef()` so the result is an owned reference. Reviewed By: mpage Differential Revision: D110910346 fbshipit-source-id: ad41ec6a96b60a1f4dc24110551821477854840c
1 parent 4b356f5 commit 90220f6

12 files changed

Lines changed: 125 additions & 15 deletions

File tree

cinderx/Jit/hir/hir.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,7 @@ bool Instr::isReplayable() const {
499499
case Opcode::kIsTruthy:
500500
case Opcode::kListAppend:
501501
case Opcode::kListExtend:
502+
case Opcode::kListSubscr:
502503
case Opcode::kLoadAttr:
503504
case Opcode::kLoadAttrCached:
504505
case Opcode::kLoadAttrSpecial:
@@ -786,6 +787,7 @@ bool isPassthrough(const Instr& instr) {
786787
case Opcode::kIsTruthy:
787788
case Opcode::kListAppend:
788789
case Opcode::kListExtend:
790+
case Opcode::kListSubscr:
789791
case Opcode::kLoadArg:
790792
case Opcode::kLoadArrayItem:
791793
case Opcode::kLoadAttr:

cinderx/Jit/hir/hir.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3383,6 +3383,23 @@ class INSTR_CLASS(
33833383
: InstrT(dst, left, right, frame) {}
33843384
};
33853385

3386+
// FT-only specialized list subscript. This is the free-threaded counterpart
3387+
// to the GIL-build `LoadArrayItem` fast path, but returns an owned reference.
3388+
class INSTR_CLASS(
3389+
ListSubscr,
3390+
(TListExact, TLongExact),
3391+
HasOutput,
3392+
Operands<2>,
3393+
DeoptBase) {
3394+
public:
3395+
ListSubscr(
3396+
Register* dst,
3397+
Register* left,
3398+
Register* right,
3399+
const FrameState& frame)
3400+
: InstrT(dst, left, right, frame) {}
3401+
};
3402+
33863403
// Return a new iterator for the object, or return it if it's an iterator
33873404
class INSTR_CLASS(GetIter, (TObject), HasOutput, Operands<1>, DeoptBase) {
33883405
public:

cinderx/Jit/hir/instr_effects.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ MemoryEffects memoryEffects(const Instr& inst) {
146146
case Opcode::kInvokeStaticFunction:
147147
case Opcode::kIsInstance:
148148
case Opcode::kIsTruthy:
149+
case Opcode::kListSubscr:
149150
case Opcode::kLoadAttr:
150151
case Opcode::kLoadAttrCached:
151152
case Opcode::kLoadAttrSpecial:
@@ -516,6 +517,7 @@ bool hasArbitraryExecution(const Instr& inst) {
516517
case Opcode::kInvokeStaticFunction:
517518
case Opcode::kIsInstance:
518519
case Opcode::kIsTruthy:
520+
case Opcode::kListSubscr:
519521
case Opcode::kLoadAttr:
520522
case Opcode::kLoadAttrCached:
521523
case Opcode::kLoadAttrSpecial:

cinderx/Jit/hir/ops.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ namespace cinderx::jit::hir {
9696
V(IsTruthy) \
9797
V(ListAppend) \
9898
V(ListExtend) \
99+
V(ListSubscr) \
99100
V(LoadArrayItem) \
100101
V(LoadFieldAddress) \
101102
V(LoadArg) \

cinderx/Jit/hir/parser.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,12 @@ HIRParser::parseInstr(std::string_view opcode, Register* dst, int bb_index) {
517517
NEW_INSTR(DictSubscr, dst, dict, key, FrameState{});
518518
break;
519519
}
520+
case Opcode::kListSubscr: {
521+
auto list = parseRegister();
522+
auto idx = parseRegister();
523+
NEW_INSTR(ListSubscr, dst, list, idx, FrameState{});
524+
break;
525+
}
520526
case Opcode::kStoreSubscr: {
521527
auto receiver = parseRegister();
522528
auto index = parseRegister();

cinderx/Jit/hir/pass.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ Type outputType(
203203
case Opcode::kImportFrom:
204204
case Opcode::kImportName:
205205
case Opcode::kInvokeIterNext:
206+
case Opcode::kListSubscr:
206207
case Opcode::kLoadAttr:
207208
case Opcode::kLoadAttrCached:
208209
case Opcode::kLoadAttrSpecial:

cinderx/Jit/hir/printer.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,7 @@ static std::string format_immediates(const Function* func, const Instr& instr) {
276276
case Opcode::kIsTruthy:
277277
case Opcode::kListAppend:
278278
case Opcode::kListExtend:
279+
case Opcode::kListSubscr:
279280
case Opcode::kLoadCellItem:
280281
case Opcode::kLoadCurrentFunc:
281282
case Opcode::kLoadFrame:

cinderx/Jit/hir/simplify.cpp

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -880,23 +880,33 @@ Register* simplifySubscript(Env& env, const BinaryOp* instr) {
880880
}
881881
}
882882

883-
// TODO(T255264263). Enable this for FT builds. See P2169673256.
884-
if (!kFreeThreadedBuild && (lhs->isA(TListExact) || lhs->isA(TTupleExact))) {
885-
Register* adjusted_idx =
886-
unboxAndCheckListOrTupleIndex(env, instr, lhs, rhs);
887-
if (adjusted_idx == nullptr) {
888-
return nullptr;
889-
}
890-
Py_ssize_t offset = offsetof(PyTupleObject, ob_item);
891-
Register* array = lhs;
892-
// Lists carry a nested array of ob_item whereas tuples are variable-sized
893-
// structs.
883+
if constexpr (kFreeThreadedBuild) {
884+
// In free-threaded builds we can't use the borrowed LoadArrayItem path
885+
// for lists because another thread may overwrite the slot at any time.
886+
// With an exact list, we can still skip PyObject_GetItem's generic
887+
// dispatch by emitting ListSubscr, which returns an owned reference
888+
// via PyList_GetItemRef.
894889
if (lhs->isA(TListExact)) {
895-
array = env.emit<LoadField>(
896-
lhs, "ob_item", offsetof(PyListObject, ob_item), TCPtr);
897-
offset = 0;
890+
return env.emit<ListSubscr>(lhs, rhs, *instr->frameState());
891+
}
892+
} else {
893+
if (lhs->isA(TListExact) || lhs->isA(TTupleExact)) {
894+
Register* adjusted_idx =
895+
unboxAndCheckListOrTupleIndex(env, instr, lhs, rhs);
896+
if (adjusted_idx == nullptr) {
897+
return nullptr;
898+
}
899+
Py_ssize_t offset = offsetof(PyTupleObject, ob_item);
900+
Register* array = lhs;
901+
// Lists carry a nested array of ob_item whereas tuples are variable-sized
902+
// structs.
903+
if (lhs->isA(TListExact)) {
904+
array = env.emit<LoadField>(
905+
lhs, "ob_item", offsetof(PyListObject, ob_item), TCPtr);
906+
offset = 0;
907+
}
908+
return env.emit<LoadArrayItem>(array, adjusted_idx, lhs, offset, TObject);
898909
}
899-
return env.emit<LoadArrayItem>(array, adjusted_idx, lhs, offset, TObject);
900910
}
901911

902912
// Unicode subscript.

cinderx/Jit/jit_rt.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2218,4 +2218,22 @@ PyObject* invokeIterNext(PyObject* iterator) {
22182218
return &iterDoneSentinel;
22192219
}
22202220

2221+
PyObject* listSubscript(
2222+
[[maybe_unused]] PyObject* list,
2223+
[[maybe_unused]] PyObject* index) {
2224+
#ifdef Py_GIL_DISABLED
2225+
Py_ssize_t i = PyNumber_AsSsize_t(index, PyExc_IndexError);
2226+
if (i == -1 && PyErr_Occurred()) {
2227+
return nullptr;
2228+
}
2229+
if (i < 0) {
2230+
// Resolve negative indices against the current size.
2231+
i += PyList_GET_SIZE(list);
2232+
}
2233+
return PyList_GetItemRef(list, i);
2234+
#else
2235+
JIT_ABORT("listSubscript is only used in free-threaded builds");
2236+
#endif
2237+
}
2238+
22212239
} // namespace cinderx::jit::rt

cinderx/Jit/jit_rt.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,6 +569,12 @@ extern PyObject iterDoneSentinel;
569569
*/
570570
PyObject* invokeIterNext(PyObject* iterator);
571571

572+
/*
573+
* FT-safe exact-list subscript used by ListSubscr. Uses PyList_GetItemRef so
574+
* the result is an owned reference.
575+
*/
576+
PyObject* listSubscript(PyObject* list, PyObject* index);
577+
572578
} // namespace cinderx::jit::rt
573579

574580
#if PY_VERSION_HEX >= 0x030D0000

0 commit comments

Comments
 (0)