From 0317c2a88f03d0ee3ba6171e1f038bfb3108a499 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Fri, 1 Apr 2022 15:36:09 +0100 Subject: [PATCH 01/10] Implement borrowing over int operations --- mypyc/irbuild/ast_helpers.py | 94 +++++++++++++++ mypyc/irbuild/builder.py | 59 +--------- mypyc/irbuild/expression.py | 59 +++++----- mypyc/irbuild/statement.py | 5 +- mypyc/lib-rt/int_ops.c | 7 +- mypyc/test-data/exceptions.test | 3 +- mypyc/test-data/irbuild-basic.test | 12 +- mypyc/test-data/irbuild-classes.test | 2 +- mypyc/test-data/irbuild-generics.test | 2 +- mypyc/test-data/refcount.test | 160 ++++++++++++++++++++++++++ 10 files changed, 306 insertions(+), 97 deletions(-) create mode 100644 mypyc/irbuild/ast_helpers.py diff --git a/mypyc/irbuild/ast_helpers.py b/mypyc/irbuild/ast_helpers.py new file mode 100644 index 000000000000..15d868f6552d --- /dev/null +++ b/mypyc/irbuild/ast_helpers.py @@ -0,0 +1,94 @@ +"""IRBuilder AST transform helpers shared between expressions and statements. + +Shared code that is tightly coupled to mypy ASTs can be put here instead of +making mypyc.irbuild.builder larger. +""" + +from mypy.nodes import ( + Expression, MemberExpr, Var, IntExpr, FloatExpr, StrExpr, BytesExpr, NameExpr, OpExpr, + UnaryExpr, ComparisonExpr, LDEF +) +from mypyc.ir.ops import BasicBlock +from mypyc.ir.rtypes import is_tagged +from mypyc.irbuild.builder import IRBuilder +from mypyc.irbuild.constant_fold import constant_fold_expr + + +def process_conditional(self: IRBuilder, e: Expression, true: BasicBlock, + false: BasicBlock) -> None: + if isinstance(e, OpExpr) and e.op in ['and', 'or']: + if e.op == 'and': + # Short circuit 'and' in a conditional context. + new = BasicBlock() + process_conditional(self, e.left, new, false) + self.activate_block(new) + process_conditional(self, e.right, true, false) + else: + # Short circuit 'or' in a conditional context. + new = BasicBlock() + process_conditional(self, e.left, true, new) + self.activate_block(new) + process_conditional(self, e.right, true, false) + elif isinstance(e, UnaryExpr) and e.op == 'not': + process_conditional(self, e.expr, false, true) + else: + res = maybe_process_conditional_comparison(self, e, true, false) + if res: + return + # Catch-all for arbitrary expressions. + reg = self.accept(e) + self.add_bool_branch(reg, true, false) + + +def maybe_process_conditional_comparison(self: IRBuilder, + e: Expression, + true: BasicBlock, + false: BasicBlock) -> bool: + """Transform simple tagged integer comparisons in a conditional context. + + Return True if the operation is supported (and was transformed). Otherwise, + do nothing and return False. + + Args: + e: Arbitrary expression + true: Branch target if comparison is true + false: Branch target if comparison is false + """ + if not isinstance(e, ComparisonExpr) or len(e.operands) != 2: + return False + ltype = self.node_type(e.operands[0]) + rtype = self.node_type(e.operands[1]) + if not is_tagged(ltype) or not is_tagged(rtype): + return False + op = e.operators[0] + if op not in ('==', '!=', '<', '<=', '>', '>='): + return False + left_expr = e.operands[0] + right_expr = e.operands[1] + borrow_left = is_borrow_friendly_expr(self, right_expr) + left = self.accept(left_expr, can_borrow=borrow_left) + right = self.accept(right_expr, can_borrow=True) + # "left op right" for two tagged integers + self.builder.compare_tagged_condition(left, right, op, true, false, e.line) + return True + + +def is_borrow_friendly_expr(self: IRBuilder, expr: Expression) -> bool: + """Can the result of the expression borrowed temporarily? + + Borrowing means keeping a reference without incrementing the reference count. + """ + if isinstance(expr, (IntExpr, FloatExpr, StrExpr, BytesExpr)): + # Literals are immportal and can always be borrowed + return True + if (isinstance(expr, (UnaryExpr, OpExpr, NameExpr, MemberExpr)) and + constant_fold_expr(self, expr) is not None): + # Literal expressions are similar to literals + return True + if isinstance(expr, NameExpr): + if isinstance(expr.node, Var) and expr.kind == LDEF: + # Local variable reference can be borrowed + return True + if isinstance(expr, MemberExpr) and self.is_native_attr_ref(expr): + return True + return False diff --git a/mypyc/irbuild/builder.py b/mypyc/irbuild/builder.py index c7ef400236b3..c27270d0de7e 100644 --- a/mypyc/irbuild/builder.py +++ b/mypyc/irbuild/builder.py @@ -21,8 +21,8 @@ from mypy.nodes import ( MypyFile, SymbolNode, Statement, OpExpr, IntExpr, NameExpr, LDEF, Var, UnaryExpr, CallExpr, IndexExpr, Expression, MemberExpr, RefExpr, Lvalue, TupleExpr, - TypeInfo, Decorator, OverloadedFuncDef, StarExpr, ComparisonExpr, GDEF, - ArgKind, ARG_POS, ARG_NAMED, FuncDef, + TypeInfo, Decorator, OverloadedFuncDef, StarExpr, ComparisonExpr, FloatExpr, StrExpr, + BytesExpr, GDEF, ArgKind, ARG_POS, ARG_NAMED, FuncDef, ) from mypy.types import ( Type, Instance, TupleType, UninhabitedType, get_proper_type @@ -915,61 +915,6 @@ def shortcircuit_expr(self, expr: OpExpr) -> Value: expr.line ) - # Conditional expressions - - def process_conditional(self, e: Expression, true: BasicBlock, false: BasicBlock) -> None: - if isinstance(e, OpExpr) and e.op in ['and', 'or']: - if e.op == 'and': - # Short circuit 'and' in a conditional context. - new = BasicBlock() - self.process_conditional(e.left, new, false) - self.activate_block(new) - self.process_conditional(e.right, true, false) - else: - # Short circuit 'or' in a conditional context. - new = BasicBlock() - self.process_conditional(e.left, true, new) - self.activate_block(new) - self.process_conditional(e.right, true, false) - elif isinstance(e, UnaryExpr) and e.op == 'not': - self.process_conditional(e.expr, false, true) - else: - res = self.maybe_process_conditional_comparison(e, true, false) - if res: - return - # Catch-all for arbitrary expressions. - reg = self.accept(e) - self.add_bool_branch(reg, true, false) - - def maybe_process_conditional_comparison(self, - e: Expression, - true: BasicBlock, - false: BasicBlock) -> bool: - """Transform simple tagged integer comparisons in a conditional context. - - Return True if the operation is supported (and was transformed). Otherwise, - do nothing and return False. - - Args: - e: Arbitrary expression - true: Branch target if comparison is true - false: Branch target if comparison is false - """ - if not isinstance(e, ComparisonExpr) or len(e.operands) != 2: - return False - ltype = self.node_type(e.operands[0]) - rtype = self.node_type(e.operands[1]) - if not is_tagged(ltype) or not is_tagged(rtype): - return False - op = e.operators[0] - if op not in ('==', '!=', '<', '<=', '>', '>='): - return False - left = self.accept(e.operands[0]) - right = self.accept(e.operands[1]) - # "left op right" for two tagged integers - self.builder.compare_tagged_condition(left, right, op, true, false, e.line) - return True - # Basic helpers def flatten_classes(self, arg: Union[RefExpr, TupleExpr]) -> Optional[List[ClassIR]]: diff --git a/mypyc/irbuild/expression.py b/mypyc/irbuild/expression.py index e1feabb0a4f3..116df489ba32 100644 --- a/mypyc/irbuild/expression.py +++ b/mypyc/irbuild/expression.py @@ -5,6 +5,7 @@ """ from typing import List, Optional, Union, Callable, cast +from typing_extensions import Final from mypy.nodes import ( Expression, NameExpr, MemberExpr, SuperExpr, CallExpr, UnaryExpr, OpExpr, IndexExpr, @@ -46,6 +47,7 @@ comprehension_helper ) from mypyc.irbuild.constant_fold import constant_fold_expr +from mypyc.irbuild.ast_helpers import is_borrow_friendly_expr, process_conditional # Name and attribute references @@ -404,6 +406,15 @@ def transform_op_expr(builder: IRBuilder, expr: OpExpr) -> Value: if folded: return folded + # Special case some int ops to allow borrowing operands. + if (is_int_rprimitive(builder.node_type(expr.left)) + and is_int_rprimitive(builder.node_type(expr.right))): + if expr.op in int_borrow_friendly_op: + borrow_left = is_borrow_friendly_expr(builder, expr.right) + left = builder.accept(expr.left, can_borrow=borrow_left) + right = builder.accept(expr.right, can_borrow=True) + return builder.binary_op(left, right, expr.op, expr.line) + return builder.binary_op( builder.accept(expr.left), builder.accept(expr.right), expr.op, expr.line ) @@ -430,26 +441,6 @@ def transform_index_expr(builder: IRBuilder, expr: IndexExpr) -> Value: base, '__getitem__', [index_reg], builder.node_type(expr), expr.line) -def is_borrow_friendly_expr(builder: IRBuilder, expr: Expression) -> bool: - """Can the result of the expression borrowed temporarily? - - Borrowing means keeping a reference without incrementing the reference count. - """ - if isinstance(expr, (IntExpr, FloatExpr, StrExpr, BytesExpr)): - # Literals are immportal and can always be borrowed - return True - if isinstance(expr, (UnaryExpr, OpExpr)) and constant_fold_expr(builder, expr) is not None: - # Literal expressions are similar to literals - return True - if isinstance(expr, NameExpr): - if isinstance(expr.node, Var) and expr.kind == LDEF: - # Local variable reference can be borrowed - return True - if isinstance(expr, MemberExpr) and builder.is_native_attr_ref(expr): - return True - return False - - def try_constant_fold(builder: IRBuilder, expr: Expression) -> Optional[Value]: """Return the constant value of an expression if possible. @@ -504,7 +495,7 @@ def try_gen_slice_op(builder: IRBuilder, base: Value, index: SliceExpr) -> Optio def transform_conditional_expr(builder: IRBuilder, expr: ConditionalExpr) -> Value: if_body, else_body, next_block = BasicBlock(), BasicBlock(), BasicBlock() - builder.process_conditional(expr.cond, if_body, else_body) + process_conditional(builder, expr.cond, if_body, else_body) expr_type = builder.node_type(expr) # Having actual Phi nodes would be really nice here! target = Register(expr_type) @@ -526,6 +517,11 @@ def transform_conditional_expr(builder: IRBuilder, expr: ConditionalExpr) -> Val return target +# These int binary operations can borrow their operands safely, since the +# primitives take this into consideration. +int_borrow_friendly_op: Final = {'+', '-', '==', '!=', '<', '<=', '>', '>='} + + def transform_comparison_expr(builder: IRBuilder, e: ComparisonExpr) -> Value: # x in (...)/[...] # x not in (...)/[...] @@ -577,11 +573,22 @@ def transform_comparison_expr(builder: IRBuilder, e: ComparisonExpr) -> Value: else: return builder.true() - if first_op in ('is', 'is not') and len(e.operators) == 1: - right = e.operands[1] - if isinstance(right, NameExpr) and right.fullname == 'builtins.None': - # Special case 'is None' / 'is not None'. - return translate_is_none(builder, e.operands[0], negated=first_op != 'is') + if len(e.operators) == 1: + # Special some common simple cases + if first_op in ('is', 'is not'): + right_expr = e.operands[1] + if isinstance(right_expr, NameExpr) and right_expr.fullname == 'builtins.None': + # Special case 'is None' / 'is not None'. + return translate_is_none(builder, e.operands[0], negated=first_op != 'is') + left_expr = e.operands[0] + if is_int_rprimitive(builder.node_type(left_expr)): + right_expr = e.operands[1] + if is_int_rprimitive(builder.node_type(right_expr)): + if first_op in int_borrow_friendly_op: + borrow_left = is_borrow_friendly_expr(builder, right_expr) + left = builder.accept(left_expr, can_borrow=borrow_left) + right = builder.accept(right_expr, can_borrow=True) + return builder.compare_tagged(left, right, first_op, e.line) # TODO: Don't produce an expression when used in conditional context # All of the trickiness here is due to support for chained conditionals diff --git a/mypyc/irbuild/statement.py b/mypyc/irbuild/statement.py index 142a77fbe946..fc7c85cc40ec 100644 --- a/mypyc/irbuild/statement.py +++ b/mypyc/irbuild/statement.py @@ -36,6 +36,7 @@ ) from mypyc.irbuild.for_helpers import for_loop_helper from mypyc.irbuild.builder import IRBuilder +from mypyc.irbuild.ast_helpers import process_conditional GenFunc = Callable[[], None] @@ -207,7 +208,7 @@ def transform_if_stmt(builder: IRBuilder, stmt: IfStmt) -> None: # If statements are normalized assert len(stmt.expr) == 1 - builder.process_conditional(stmt.expr[0], if_body, else_body) + process_conditional(builder, stmt.expr[0], if_body, else_body) builder.activate_block(if_body) builder.accept(stmt.body[0]) builder.goto(next) @@ -226,7 +227,7 @@ def transform_while_stmt(builder: IRBuilder, s: WhileStmt) -> None: # Split block so that we get a handle to the top of the loop. builder.goto_and_activate(top) - builder.process_conditional(s.expr, body, normal_loop_exit) + process_conditional(builder, s.expr, body, normal_loop_exit) builder.activate_block(body) builder.accept(s.body) diff --git a/mypyc/lib-rt/int_ops.c b/mypyc/lib-rt/int_ops.c index edf063141619..caf0fe0b5391 100644 --- a/mypyc/lib-rt/int_ops.c +++ b/mypyc/lib-rt/int_ops.c @@ -250,8 +250,11 @@ bool CPyTagged_IsEq_(CPyTagged left, CPyTagged right) { if (CPyTagged_CheckShort(right)) { return false; } else { - int result = PyObject_RichCompareBool(CPyTagged_LongAsObject(left), - CPyTagged_LongAsObject(right), Py_EQ); + PyObject *left_obj = CPyTagged_AsObject(left); + PyObject *right_obj = CPyTagged_AsObject(right); + int result = PyObject_RichCompareBool(left_obj, right_obj, Py_EQ); + Py_DECREF(left_obj); + Py_DECREF(right_obj); if (result == -1) { CPyError_OutOfMemory(); } diff --git a/mypyc/test-data/exceptions.test b/mypyc/test-data/exceptions.test index 8c576b49ce82..8b186e234c5e 100644 --- a/mypyc/test-data/exceptions.test +++ b/mypyc/test-data/exceptions.test @@ -135,11 +135,10 @@ L4: r5 = i < l :: signed if r5 goto L5 else goto L10 :: bool L5: - r6 = CPyList_GetItem(a, i) + r6 = CPyList_GetItemBorrow(a, i) if is_error(r6) goto L11 (error at sum:6) else goto L6 L6: r7 = unbox(int, r6) - dec_ref r6 if is_error(r7) goto L11 (error at sum:6) else goto L7 L7: r8 = CPyTagged_Add(sum, r7) diff --git a/mypyc/test-data/irbuild-basic.test b/mypyc/test-data/irbuild-basic.test index 077abcf2939b..37f387c3e46c 100644 --- a/mypyc/test-data/irbuild-basic.test +++ b/mypyc/test-data/irbuild-basic.test @@ -2197,14 +2197,14 @@ L0: r0 = self.is_add if r0 goto L1 else goto L2 :: bool L1: - r1 = self.left - r2 = self.right + r1 = borrow self.left + r2 = borrow self.right r3 = CPyTagged_Add(r1, r2) r4 = r3 goto L3 L2: - r5 = self.left - r6 = self.right + r5 = borrow self.left + r6 = borrow self.right r7 = CPyTagged_Subtract(r5, r6) r4 = r7 L3: @@ -2292,7 +2292,7 @@ def BaseProperty.next(self): r0, r1 :: int r2 :: __main__.BaseProperty L0: - r0 = self._incrementer + r0 = borrow self._incrementer r1 = CPyTagged_Add(r0, 2) r2 = BaseProperty(r1) return r2 @@ -2483,7 +2483,7 @@ def g(a, b, c): r2, r3 :: int L0: r0 = a.__getitem__(c) - r1 = CPyList_GetItem(b, c) + r1 = CPyList_GetItemBorrow(b, c) r2 = unbox(int, r1) r3 = CPyTagged_Add(r0, r2) return r3 diff --git a/mypyc/test-data/irbuild-classes.test b/mypyc/test-data/irbuild-classes.test index fcf6ef957435..e194214423e7 100644 --- a/mypyc/test-data/irbuild-classes.test +++ b/mypyc/test-data/irbuild-classes.test @@ -59,7 +59,7 @@ L0: r5 = CPyList_GetItemShort(a, 0) r6 = cast(__main__.C, r5) d = r6 - r7 = d.x + r7 = borrow d.x r8 = CPyTagged_Add(r7, 2) return r8 diff --git a/mypyc/test-data/irbuild-generics.test b/mypyc/test-data/irbuild-generics.test index 10f8e737d639..64a03c9444e2 100644 --- a/mypyc/test-data/irbuild-generics.test +++ b/mypyc/test-data/irbuild-generics.test @@ -62,7 +62,7 @@ L0: c = r0 r1 = object 1 c.x = r1; r2 = is_error - r3 = c.x + r3 = borrow c.x r4 = unbox(int, r3) r5 = CPyTagged_Add(4, r4) return 1 diff --git a/mypyc/test-data/refcount.test b/mypyc/test-data/refcount.test index a7ee390c8d74..d0a5c7aa0788 100644 --- a/mypyc/test-data/refcount.test +++ b/mypyc/test-data/refcount.test @@ -1241,3 +1241,163 @@ L0: r0 = borrow x.c r0.b = 0; r1 = is_error return 1 + +[case testBorrowIntEquality] +def add(c: C) -> bool: + return c.x == c.y + +class C: + x: int + y: int +[out] +def add(c): + c :: __main__.C + r0, r1 :: int + r2 :: native_int + r3, r4 :: bit + r5 :: bool + r6 :: bit +L0: + r0 = borrow c.x + r1 = borrow c.y + r2 = r0 & 1 + r3 = r2 == 0 + if r3 goto L1 else goto L2 :: bool +L1: + r4 = r0 == r1 + r5 = r4 + goto L3 +L2: + r6 = CPyTagged_IsEq_(r0, r1) + r5 = r6 +L3: + return r5 + +[case testBorrowIntLessThan] +def add(c: C) -> bool: + return c.x < c.y + +class C: + x: int + y: int +[out] +def add(c): + c :: __main__.C + r0, r1 :: int + r2 :: native_int + r3 :: bit + r4 :: native_int + r5, r6, r7 :: bit + r8 :: bool + r9 :: bit +L0: + r0 = borrow c.x + r1 = borrow c.y + r2 = r0 & 1 + r3 = r2 == 0 + r4 = r1 & 1 + r5 = r4 == 0 + r6 = r3 & r5 + if r6 goto L1 else goto L2 :: bool +L1: + r7 = r0 < r1 :: signed + r8 = r7 + goto L3 +L2: + r9 = CPyTagged_IsLt_(r0, r1) + r8 = r9 +L3: + return r8 + +[case testBorrowIntCompareFinal] +from typing_extensions import Final + +X: Final = 10 + +def add(c: C) -> bool: + return c.x == X + +class C: + x: int +[out] +def add(c): + c :: __main__.C + r0 :: int + r1 :: native_int + r2, r3 :: bit + r4 :: bool + r5 :: bit +L0: + r0 = borrow c.x + r1 = r0 & 1 + r2 = r1 == 0 + if r2 goto L1 else goto L2 :: bool +L1: + r3 = r0 == 20 + r4 = r3 + goto L3 +L2: + r5 = CPyTagged_IsEq_(r0, 20) + r4 = r5 +L3: + return r4 + +[case testBorrowIntArithmetic] +def add(c: C) -> int: + return c.x + c.y + +def sub(c: C) -> int: + return c.x - c.y + +class C: + x: int + y: int +[out] +def add(c): + c :: __main__.C + r0, r1, r2 :: int +L0: + r0 = borrow c.x + r1 = borrow c.y + r2 = CPyTagged_Add(r0, r1) + return r2 +def sub(c): + c :: __main__.C + r0, r1, r2 :: int +L0: + r0 = borrow c.x + r1 = borrow c.y + r2 = CPyTagged_Subtract(r0, r1) + return r2 + +[case testBorrowIntComparisonInIf] +def add(c: C, n: int) -> bool: + if c.x == c.y: + return True + return False + +class C: + x: int + y: int +[out] +def add(c, n): + c :: __main__.C + n, r0, r1 :: int + r2 :: native_int + r3, r4, r5 :: bit +L0: + r0 = borrow c.x + r1 = borrow c.y + r2 = r0 & 1 + r3 = r2 != 0 + if r3 goto L1 else goto L2 :: bool +L1: + r4 = CPyTagged_IsEq_(r0, r1) + if r4 goto L3 else goto L4 :: bool +L2: + r5 = r0 == r1 + if r5 goto L3 else goto L4 :: bool +L3: + return 1 +L4: + return 0 From 2221ae4507697c54d983ace50f9c7e18261f2c08 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Fri, 1 Apr 2022 17:21:14 +0100 Subject: [PATCH 02/10] Support borrowing in x.y += n etc. --- mypyc/irbuild/builder.py | 17 +++++++++++---- mypyc/irbuild/expression.py | 8 +------ mypyc/irbuild/statement.py | 17 ++++++++++----- mypyc/irbuild/targets.py | 3 ++- mypyc/test-data/irbuild-classes.test | 2 +- mypyc/test-data/refcount.test | 31 ++++++++++++++++++++++++++++ 6 files changed, 60 insertions(+), 18 deletions(-) diff --git a/mypyc/irbuild/builder.py b/mypyc/irbuild/builder.py index c27270d0de7e..393526041be7 100644 --- a/mypyc/irbuild/builder.py +++ b/mypyc/irbuild/builder.py @@ -14,7 +14,7 @@ from mypyc.irbuild.prepare import RegisterImplInfo from typing import Callable, Dict, List, Tuple, Optional, Union, Sequence, Set, Any, Iterator -from typing_extensions import overload +from typing_extensions import overload, Final from mypy.backports import OrderedDict from mypy.build import Graph @@ -67,6 +67,11 @@ from mypyc.irbuild.util import is_constant +# These int binary operations can borrow their operands safely, since the +# primitives take this into consideration. +int_borrow_friendly_op: Final = {'+', '-', '==', '!=', '<', '<=', '>', '>='} + + class IRVisitor(ExpressionVisitor[Value], StatementVisitor[None]): pass @@ -515,7 +520,7 @@ def get_assignment_target(self, lvalue: Lvalue, # Attribute assignment x.y = e can_borrow = self.is_native_attr_ref(lvalue) obj = self.accept(lvalue.expr, can_borrow=can_borrow) - return AssignmentTargetAttr(obj, lvalue.name) + return AssignmentTargetAttr(obj, lvalue.name, can_borrow=can_borrow) elif isinstance(lvalue, TupleExpr): # Multiple assignment a, ..., b = e star_idx: Optional[int] = None @@ -535,7 +540,10 @@ def get_assignment_target(self, lvalue: Lvalue, assert False, 'Unsupported lvalue: %r' % lvalue - def read(self, target: Union[Value, AssignmentTarget], line: int = -1) -> Value: + def read(self, + target: Union[Value, AssignmentTarget], + line: int = -1, + can_borrow: bool = False) -> Value: if isinstance(target, Value): return target if isinstance(target, AssignmentTargetRegister): @@ -548,7 +556,8 @@ def read(self, target: Union[Value, AssignmentTarget], line: int = -1) -> Value: assert False, target.base.type if isinstance(target, AssignmentTargetAttr): if isinstance(target.obj.type, RInstance) and target.obj.type.class_ir.is_ext_class: - return self.add(GetAttr(target.obj, target.attr, line)) + borrow = can_borrow and target.can_borrow + return self.add(GetAttr(target.obj, target.attr, line, borrow=borrow)) else: return self.py_get_attr(target.obj, target.attr, line) diff --git a/mypyc/irbuild/expression.py b/mypyc/irbuild/expression.py index 116df489ba32..76e4db62a465 100644 --- a/mypyc/irbuild/expression.py +++ b/mypyc/irbuild/expression.py @@ -5,7 +5,6 @@ """ from typing import List, Optional, Union, Callable, cast -from typing_extensions import Final from mypy.nodes import ( Expression, NameExpr, MemberExpr, SuperExpr, CallExpr, UnaryExpr, OpExpr, IndexExpr, @@ -41,7 +40,7 @@ from mypyc.primitives.str_ops import str_slice_op from mypyc.primitives.int_ops import int_comparison_op_mapping from mypyc.irbuild.specialize import apply_function_specialization, apply_method_specialization -from mypyc.irbuild.builder import IRBuilder +from mypyc.irbuild.builder import IRBuilder, int_borrow_friendly_op from mypyc.irbuild.for_helpers import ( translate_list_comprehension, translate_set_comprehension, comprehension_helper @@ -517,11 +516,6 @@ def transform_conditional_expr(builder: IRBuilder, expr: ConditionalExpr) -> Val return target -# These int binary operations can borrow their operands safely, since the -# primitives take this into consideration. -int_borrow_friendly_op: Final = {'+', '-', '==', '!=', '<', '<=', '>', '>='} - - def transform_comparison_expr(builder: IRBuilder, e: ComparisonExpr) -> Value: # x in (...)/[...] # x not in (...)/[...] diff --git a/mypyc/irbuild/statement.py b/mypyc/irbuild/statement.py index fc7c85cc40ec..93dc5f24158f 100644 --- a/mypyc/irbuild/statement.py +++ b/mypyc/irbuild/statement.py @@ -20,7 +20,7 @@ Assign, Unreachable, RaiseStandardError, LoadErrorValue, BasicBlock, TupleGet, Value, Register, Branch, NO_TRACEBACK_LINE_NO ) -from mypyc.ir.rtypes import RInstance, exc_rtuple +from mypyc.ir.rtypes import RInstance, exc_rtuple, is_tagged from mypyc.primitives.generic_ops import py_delattr_op from mypyc.primitives.misc_ops import type_op, import_from_op from mypyc.primitives.exc_ops import ( @@ -35,8 +35,8 @@ ExceptNonlocalControl, FinallyNonlocalControl, TryFinallyNonlocalControl ) from mypyc.irbuild.for_helpers import for_loop_helper -from mypyc.irbuild.builder import IRBuilder -from mypyc.irbuild.ast_helpers import process_conditional +from mypyc.irbuild.builder import IRBuilder, int_borrow_friendly_op +from mypyc.irbuild.ast_helpers import process_conditional, is_borrow_friendly_expr GenFunc = Callable[[], None] @@ -120,9 +120,16 @@ def is_simple_lvalue(expr: Expression) -> bool: def transform_operator_assignment_stmt(builder: IRBuilder, stmt: OperatorAssignmentStmt) -> None: """Operator assignment statement such as x += 1""" builder.disallow_class_assignments([stmt.lvalue], stmt.line) + if (is_tagged(builder.node_type(stmt.lvalue)) + and is_tagged(builder.node_type(stmt.rvalue)) + and stmt.op in int_borrow_friendly_op): + can_borrow = (is_borrow_friendly_expr(builder, stmt.rvalue) + and is_borrow_friendly_expr(builder, stmt.lvalue)) + else: + can_borrow = False target = builder.get_assignment_target(stmt.lvalue) - target_value = builder.read(target, stmt.line) - rreg = builder.accept(stmt.rvalue) + target_value = builder.read(target, stmt.line, can_borrow=can_borrow) + rreg = builder.accept(stmt.rvalue, can_borrow=can_borrow) # the Python parser strips the '=' from operator assignment statements, so re-add it op = stmt.op + '=' res = builder.binary_op(target_value, rreg, op, stmt.line) diff --git a/mypyc/irbuild/targets.py b/mypyc/irbuild/targets.py index f6346d4fa7e7..f2daa701f7e8 100644 --- a/mypyc/irbuild/targets.py +++ b/mypyc/irbuild/targets.py @@ -35,9 +35,10 @@ def __init__(self, base: Value, index: Value) -> None: class AssignmentTargetAttr(AssignmentTarget): """obj.attr as assignment target""" - def __init__(self, obj: Value, attr: str) -> None: + def __init__(self, obj: Value, attr: str, can_borrow: bool = False) -> None: self.obj = obj self.attr = attr + self.can_borrow = can_borrow if isinstance(obj.type, RInstance) and obj.type.class_ir.has_attr(attr): # Native attribute reference self.obj_type: RType = obj.type diff --git a/mypyc/test-data/irbuild-classes.test b/mypyc/test-data/irbuild-classes.test index e194214423e7..9c3dc4508ae8 100644 --- a/mypyc/test-data/irbuild-classes.test +++ b/mypyc/test-data/irbuild-classes.test @@ -175,7 +175,7 @@ def increment(o): r0, r1 :: int r2 :: bool L0: - r0 = o.x + r0 = borrow o.x r1 = CPyTagged_Add(r0, 2) o.x = r1; r2 = is_error return o diff --git a/mypyc/test-data/refcount.test b/mypyc/test-data/refcount.test index d0a5c7aa0788..6179cb2766b1 100644 --- a/mypyc/test-data/refcount.test +++ b/mypyc/test-data/refcount.test @@ -1401,3 +1401,34 @@ L3: return 1 L4: return 0 + +[case testBorrowIntInPlaceOp] +def add(c: C, n: int) -> None: + c.x += n + +def sub(c: C, n: int) -> None: + c.x -= c.y + +class C: + x: int + y: int +[out] +def add(c, n): + c :: __main__.C + n, r0, r1 :: int + r2 :: bool +L0: + r0 = borrow c.x + r1 = CPyTagged_Add(r0, n) + c.x = r1; r2 = is_error + return 1 +def sub(c, n): + c :: __main__.C + n, r0, r1, r2 :: int + r3 :: bool +L0: + r0 = borrow c.x + r1 = borrow c.y + r2 = CPyTagged_Subtract(r0, r1) + c.x = r2; r3 = is_error + return 1 From a06c32a43e47c59e184fd4689b53997808febed4 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Fri, 1 Apr 2022 11:52:47 +0100 Subject: [PATCH 03/10] Support borrowed list get item ops --- mypyc/irbuild/builder.py | 2 +- mypyc/irbuild/ll_builder.py | 21 +++++++++------ mypyc/lib-rt/CPy.h | 2 ++ mypyc/lib-rt/list_ops.c | 41 ++++++++++++++++++++++++++++++ mypyc/primitives/list_ops.py | 22 +++++++++++++++- mypyc/test-data/irbuild-lists.test | 4 +-- mypyc/test-data/refcount.test | 31 ++++++++++++++++++++-- 7 files changed, 109 insertions(+), 14 deletions(-) diff --git a/mypyc/irbuild/builder.py b/mypyc/irbuild/builder.py index 393526041be7..2a53be15df95 100644 --- a/mypyc/irbuild/builder.py +++ b/mypyc/irbuild/builder.py @@ -292,7 +292,7 @@ def gen_method_call(self, arg_kinds: Optional[List[ArgKind]] = None, arg_names: Optional[List[Optional[str]]] = None) -> Value: return self.builder.gen_method_call( - base, name, arg_values, result_type, line, arg_kinds, arg_names + base, name, arg_values, result_type, line, arg_kinds, arg_names, self.can_borrow ) def load_module(self, name: str) -> Value: diff --git a/mypyc/irbuild/ll_builder.py b/mypyc/irbuild/ll_builder.py index c7d8dc7b3ab2..67095c93c801 100644 --- a/mypyc/irbuild/ll_builder.py +++ b/mypyc/irbuild/ll_builder.py @@ -725,7 +725,8 @@ def gen_method_call(self, result_type: Optional[RType], line: int, arg_kinds: Optional[List[ArgKind]] = None, - arg_names: Optional[List[Optional[str]]] = None) -> Value: + arg_names: Optional[List[Optional[str]]] = None, + can_borrow: bool = False) -> Value: """Generate either a native or Python method call.""" # If we have *args, then fallback to Python method call. if arg_kinds is not None and any(kind.is_star() for kind in arg_kinds): @@ -759,7 +760,8 @@ def gen_method_call(self, # Try to do a special-cased method call if not arg_kinds or arg_kinds == [ARG_POS] * len(arg_values): - target = self.translate_special_method_call(base, name, arg_values, result_type, line) + target = self.translate_special_method_call( + base, name, arg_values, result_type, line, can_borrow=can_borrow) if target: return target @@ -1332,20 +1334,22 @@ def call_c(self, # and so we can't just coerce it. result = self.none() else: - result = self.coerce(target, result_type, line) + result = self.coerce(target, result_type, line, can_borrow=desc.is_borrowed) return result def matching_call_c(self, candidates: List[CFunctionDescription], args: List[Value], line: int, - result_type: Optional[RType] = None) -> Optional[Value]: + result_type: Optional[RType] = None, + can_borrow: bool = False) -> Optional[Value]: matching: Optional[CFunctionDescription] = None for desc in candidates: if len(desc.arg_types) != len(args): continue - if all(is_subtype(actual.type, formal) - for actual, formal in zip(args, desc.arg_types)): + if (all(is_subtype(actual.type, formal) + for actual, formal in zip(args, desc.arg_types)) and + (not desc.is_borrowed or can_borrow)): if matching: assert matching.priority != desc.priority, 'Ambiguous:\n1) {}\n2) {}'.format( matching, desc) @@ -1500,7 +1504,8 @@ def translate_special_method_call(self, name: str, args: List[Value], result_type: Optional[RType], - line: int) -> Optional[Value]: + line: int, + can_borrow: bool = False) -> Optional[Value]: """Translate a method call which is handled nongenerically. These are special in the sense that we have code generated specifically for them. @@ -1511,7 +1516,7 @@ def translate_special_method_call(self, """ call_c_ops_candidates = method_call_ops.get(name, []) call_c_op = self.matching_call_c(call_c_ops_candidates, [base_reg] + args, - line, result_type) + line, result_type, can_borrow=can_borrow) return call_c_op def translate_eq_cmp(self, diff --git a/mypyc/lib-rt/CPy.h b/mypyc/lib-rt/CPy.h index 4c0f91a5707e..0fdd6b0a27cc 100644 --- a/mypyc/lib-rt/CPy.h +++ b/mypyc/lib-rt/CPy.h @@ -340,6 +340,8 @@ PyObject *CPyList_Build(Py_ssize_t len, ...); PyObject *CPyList_GetItem(PyObject *list, CPyTagged index); PyObject *CPyList_GetItemUnsafe(PyObject *list, CPyTagged index); PyObject *CPyList_GetItemShort(PyObject *list, CPyTagged index); +PyObject *CPyList_GetItemBorrow(PyObject *list, CPyTagged index); +PyObject *CPyList_GetItemShortBorrow(PyObject *list, CPyTagged index); bool CPyList_SetItem(PyObject *list, CPyTagged index, PyObject *value); bool CPyList_SetItemUnsafe(PyObject *list, CPyTagged index, PyObject *value); PyObject *CPyList_PopLast(PyObject *obj); diff --git a/mypyc/lib-rt/list_ops.c b/mypyc/lib-rt/list_ops.c index 28547cfd7b60..885c1a3366f3 100644 --- a/mypyc/lib-rt/list_ops.c +++ b/mypyc/lib-rt/list_ops.c @@ -52,6 +52,24 @@ PyObject *CPyList_GetItemShort(PyObject *list, CPyTagged index) { return result; } +PyObject *CPyList_GetItemShortBorrow(PyObject *list, CPyTagged index) { + Py_ssize_t n = CPyTagged_ShortAsSsize_t(index); + Py_ssize_t size = PyList_GET_SIZE(list); + if (n >= 0) { + if (n >= size) { + PyErr_SetString(PyExc_IndexError, "list index out of range"); + return NULL; + } + } else { + n += size; + if (n < 0) { + PyErr_SetString(PyExc_IndexError, "list index out of range"); + return NULL; + } + } + return PyList_GET_ITEM(list, n); +} + PyObject *CPyList_GetItem(PyObject *list, CPyTagged index) { if (CPyTagged_CheckShort(index)) { Py_ssize_t n = CPyTagged_ShortAsSsize_t(index); @@ -77,6 +95,29 @@ PyObject *CPyList_GetItem(PyObject *list, CPyTagged index) { } } +PyObject *CPyList_GetItemBorrow(PyObject *list, CPyTagged index) { + if (CPyTagged_CheckShort(index)) { + Py_ssize_t n = CPyTagged_ShortAsSsize_t(index); + Py_ssize_t size = PyList_GET_SIZE(list); + if (n >= 0) { + if (n >= size) { + PyErr_SetString(PyExc_IndexError, "list index out of range"); + return NULL; + } + } else { + n += size; + if (n < 0) { + PyErr_SetString(PyExc_IndexError, "list index out of range"); + return NULL; + } + } + return PyList_GET_ITEM(list, n); + } else { + PyErr_SetString(PyExc_OverflowError, CPYTHON_LARGE_INT_ERRMSG); + return NULL; + } +} + bool CPyList_SetItem(PyObject *list, CPyTagged index, PyObject *value) { if (CPyTagged_CheckShort(index)) { Py_ssize_t n = CPyTagged_ShortAsSsize_t(index); diff --git a/mypyc/primitives/list_ops.py b/mypyc/primitives/list_ops.py index 3988511c9772..78955f70f164 100644 --- a/mypyc/primitives/list_ops.py +++ b/mypyc/primitives/list_ops.py @@ -55,7 +55,7 @@ c_function_name='CPyList_GetItem', error_kind=ERR_MAGIC) -# Version with no int bounds check for when it is known to be short +# list[index] version with no int bounds check for when it is known to be short method_op( name='__getitem__', arg_types=[list_rprimitive, short_int_rprimitive], @@ -64,6 +64,26 @@ error_kind=ERR_MAGIC, priority=2) +# list[index] that produces a borrowed result +method_op( + name='__getitem__', + arg_types=[list_rprimitive, int_rprimitive], + return_type=object_rprimitive, + c_function_name='CPyList_GetItemBorrow', + error_kind=ERR_MAGIC, + is_borrowed=True, + priority=3) + +# list[index] that produces a borrowed result and index is known to be short +method_op( + name='__getitem__', + arg_types=[list_rprimitive, short_int_rprimitive], + return_type=object_rprimitive, + c_function_name='CPyList_GetItemShortBorrow', + error_kind=ERR_MAGIC, + is_borrowed=True, + priority=4) + # This is unsafe because it assumes that the index is a non-negative short integer # that is in-bounds for the list. list_get_item_unsafe_op = custom_op( diff --git a/mypyc/test-data/irbuild-lists.test b/mypyc/test-data/irbuild-lists.test index 3173469c8db6..4a906089f68a 100644 --- a/mypyc/test-data/irbuild-lists.test +++ b/mypyc/test-data/irbuild-lists.test @@ -38,8 +38,8 @@ def f(x): r2 :: object r3 :: int L0: - r0 = CPyList_GetItemShort(x, 0) - r1 = cast(list, r0) + r0 = CPyList_GetItemShortBorrow(x, 0) + r1 = borrow cast(list, r0) r2 = CPyList_GetItemShort(r1, 2) r3 = unbox(int, r2) return r3 diff --git a/mypyc/test-data/refcount.test b/mypyc/test-data/refcount.test index 6179cb2766b1..3887c5534f04 100644 --- a/mypyc/test-data/refcount.test +++ b/mypyc/test-data/refcount.test @@ -1153,14 +1153,20 @@ L0: [case testBorrowListGetItem2] from typing import List -def attr_index(x: C) -> str: +def attr_before_index(x: C) -> str: return x.a[x.n] +def attr_after_index(a: List[C], i: int) -> int: + return a[i].n + +def attr_after_index_literal(a: List[C]) -> int: + return a[0].n + class C: a: List[str] n: int [out] -def attr_index(x): +def attr_before_index(x): x :: __main__.C r0 :: list r1 :: int @@ -1172,6 +1178,27 @@ L0: r2 = CPyList_GetItem(r0, r1) r3 = cast(str, r2) return r3 +def attr_after_index(a, i): + a :: list + i :: int + r0 :: object + r1 :: __main__.C + r2 :: int +L0: + r0 = CPyList_GetItemBorrow(a, i) + r1 = borrow cast(__main__.C, r0) + r2 = r1.n + return r2 +def attr_after_index_literal(a): + a :: list + r0 :: object + r1 :: __main__.C + r2 :: int +L0: + r0 = CPyList_GetItemShortBorrow(a, 0) + r1 = borrow cast(__main__.C, r0) + r2 = r1.n + return r2 [case testCannotBorrowListGetItem] from typing import List From 298f145698692f1bf3648e58d5c0331890874a39 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Sat, 2 Apr 2022 11:07:15 +0100 Subject: [PATCH 04/10] Fix lifetimes of borrowed list items Fixes segfaults. --- mypyc/irbuild/ll_builder.py | 7 +++++++ mypyc/test-data/refcount.test | 31 +++++++++++++++++++++++++++++++ mypyc/test-data/run-lists.test | 25 +++++++++++++++++++++++++ 3 files changed, 63 insertions(+) diff --git a/mypyc/irbuild/ll_builder.py b/mypyc/irbuild/ll_builder.py index 67095c93c801..310f28d9d696 100644 --- a/mypyc/irbuild/ll_builder.py +++ b/mypyc/irbuild/ll_builder.py @@ -1315,6 +1315,13 @@ def call_c(self, error_kind = ERR_NEVER target = self.add(CallC(desc.c_function_name, coerced, desc.return_type, desc.steals, desc.is_borrowed, error_kind, line, var_arg_idx)) + if desc.is_borrowed: + # If the result is borrowed, force the arguments to be + # kept alive afterwards, as otherwise the result might be + # immediately freed, at the risk of a dangling pointer. + for arg in coerced: + if not isinstance(arg, (Integer, LoadLiteral)): + self.keep_alives.append(arg) if desc.error_kind == ERR_NEG_INT: comp = ComparisonOp(target, Integer(0, desc.return_type, line), diff --git a/mypyc/test-data/refcount.test b/mypyc/test-data/refcount.test index 3887c5534f04..ce365fc50e7e 100644 --- a/mypyc/test-data/refcount.test +++ b/mypyc/test-data/refcount.test @@ -1229,6 +1229,37 @@ def f(): L0: return 0 +[case testBorrowListGetItemKeepAlive] +from typing import List + +def f() -> str: + a = [C()] + return a[0].s + +class C: + s: str +[out] +def f(): + r0 :: __main__.C + r1 :: list + r2, r3 :: ptr + a :: list + r4 :: object + r5 :: __main__.C + r6 :: str +L0: + r0 = C() + r1 = PyList_New(1) + r2 = get_element_ptr r1 ob_item :: PyListObject + r3 = load_mem r2 :: ptr* + set_mem r3, r0 :: builtins.object* + a = r1 + r4 = CPyList_GetItemShortBorrow(a, 0) + r5 = borrow cast(__main__.C, r4) + r6 = r5.s + dec_ref a + return r6 + [case testBorrowSetAttrObject] from typing import Optional diff --git a/mypyc/test-data/run-lists.test b/mypyc/test-data/run-lists.test index c98ab9171123..84d5ee121a20 100644 --- a/mypyc/test-data/run-lists.test +++ b/mypyc/test-data/run-lists.test @@ -379,8 +379,33 @@ def test() -> None: source_str = "abcd" f = list("str:" + x for x in source_str) assert f == ["str:a", "str:b", "str:c", "str:d"] + [case testNextBug] from typing import List, Optional def test(x: List[int]) -> None: res = next((i for i in x), None) + +[case testListGetItemWithBorrow] +from typing import List + +class D: + def __init__(self, n: int) -> None: + self.n = n + +class C: + def __init__(self, d: D) -> None: + self.d = d + +def test_index_with_literal() -> None: + d1 = D(1) + d2 = D(2) + a = [C(d1), C(d2)] + d = a[0].d + assert d is d1 + d = a[1].d + assert d is d2 + d = a[-1].d + assert d is d2 + d = a[-2].d + assert d is d1 From b734542da3e420ca3545d66c7824555f1c6f9707 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Sun, 3 Apr 2022 10:22:03 +0100 Subject: [PATCH 05/10] Update test cases to use keep_alive --- mypyc/test-data/irbuild-basic.test | 3 +++ mypyc/test-data/irbuild-classes.test | 1 + 2 files changed, 4 insertions(+) diff --git a/mypyc/test-data/irbuild-basic.test b/mypyc/test-data/irbuild-basic.test index 37f387c3e46c..157f6cdf0786 100644 --- a/mypyc/test-data/irbuild-basic.test +++ b/mypyc/test-data/irbuild-basic.test @@ -2200,12 +2200,14 @@ L1: r1 = borrow self.left r2 = borrow self.right r3 = CPyTagged_Add(r1, r2) + keep_alive self, self r4 = r3 goto L3 L2: r5 = borrow self.left r6 = borrow self.right r7 = CPyTagged_Subtract(r5, r6) + keep_alive self, self r4 = r7 L3: return r4 @@ -2294,6 +2296,7 @@ def BaseProperty.next(self): L0: r0 = borrow self._incrementer r1 = CPyTagged_Add(r0, 2) + keep_alive self r2 = BaseProperty(r1) return r2 def BaseProperty.__init__(self, value): diff --git a/mypyc/test-data/irbuild-classes.test b/mypyc/test-data/irbuild-classes.test index 9c3dc4508ae8..9fe5aba3bc60 100644 --- a/mypyc/test-data/irbuild-classes.test +++ b/mypyc/test-data/irbuild-classes.test @@ -61,6 +61,7 @@ L0: d = r6 r7 = borrow d.x r8 = CPyTagged_Add(r7, 2) + keep_alive d return r8 [case testMethodCall] From ad0d862d62d8004cc8bb98f68dddb4476f90d2a0 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Fri, 15 Apr 2022 12:47:18 +0100 Subject: [PATCH 06/10] Fix lint --- mypyc/irbuild/builder.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mypyc/irbuild/builder.py b/mypyc/irbuild/builder.py index 2a53be15df95..c1662d2fdac2 100644 --- a/mypyc/irbuild/builder.py +++ b/mypyc/irbuild/builder.py @@ -21,8 +21,8 @@ from mypy.nodes import ( MypyFile, SymbolNode, Statement, OpExpr, IntExpr, NameExpr, LDEF, Var, UnaryExpr, CallExpr, IndexExpr, Expression, MemberExpr, RefExpr, Lvalue, TupleExpr, - TypeInfo, Decorator, OverloadedFuncDef, StarExpr, ComparisonExpr, FloatExpr, StrExpr, - BytesExpr, GDEF, ArgKind, ARG_POS, ARG_NAMED, FuncDef, + TypeInfo, Decorator, OverloadedFuncDef, StarExpr, + GDEF, ArgKind, ARG_POS, ARG_NAMED, FuncDef, ) from mypy.types import ( Type, Instance, TupleType, UninhabitedType, get_proper_type @@ -40,7 +40,7 @@ from mypyc.ir.rtypes import ( RType, RTuple, RInstance, c_int_rprimitive, int_rprimitive, dict_rprimitive, none_rprimitive, is_none_rprimitive, object_rprimitive, is_object_rprimitive, - str_rprimitive, is_tagged, is_list_rprimitive, is_tuple_rprimitive, c_pyssize_t_rprimitive + str_rprimitive, is_list_rprimitive, is_tuple_rprimitive, c_pyssize_t_rprimitive ) from mypyc.ir.func_ir import FuncIR, INVALID_FUNC_DEF, RuntimeArg, FuncSignature, FuncDecl from mypyc.ir.class_ir import ClassIR, NonExtClassInfo From d66c3843c74ee94bb9bfa34b8ce565906002f824 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Fri, 15 Apr 2022 12:50:47 +0100 Subject: [PATCH 07/10] Fix tests --- mypyc/test-data/irbuild-basic.test | 1 + 1 file changed, 1 insertion(+) diff --git a/mypyc/test-data/irbuild-basic.test b/mypyc/test-data/irbuild-basic.test index 157f6cdf0786..8e54b25b673b 100644 --- a/mypyc/test-data/irbuild-basic.test +++ b/mypyc/test-data/irbuild-basic.test @@ -2489,6 +2489,7 @@ L0: r1 = CPyList_GetItemBorrow(b, c) r2 = unbox(int, r1) r3 = CPyTagged_Add(r0, r2) + keep_alive b, c return r3 [case testTypeAlias_toplevel] From dd657460e84a6592c8d9f193511fc31c44c92704 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Mon, 18 Apr 2022 14:39:48 +0100 Subject: [PATCH 08/10] Flush keep alives before taking a branch --- mypyc/irbuild/ll_builder.py | 4 +++ mypyc/test-data/irbuild-classes.test | 44 ++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/mypyc/irbuild/ll_builder.py b/mypyc/irbuild/ll_builder.py index 310f28d9d696..d5154707538b 100644 --- a/mypyc/irbuild/ll_builder.py +++ b/mypyc/irbuild/ll_builder.py @@ -970,12 +970,14 @@ def compare_tagged_condition(self, is_short_int_rprimitive(rhs.type)))): # We can skip the tag check check = self.comparison_op(lhs, rhs, int_comparison_op_mapping[op][0], line) + self.flush_keep_alives() self.add(Branch(check, true, false, Branch.BOOL)) return op_type, c_func_desc, negate_result, swap_op = int_comparison_op_mapping[op] int_block, short_int_block = BasicBlock(), BasicBlock() check_lhs = self.check_tagged_short_int(lhs, line, negated=True) if is_eq or is_short_int_rprimitive(rhs.type): + self.flush_keep_alives() self.add(Branch(check_lhs, int_block, short_int_block, Branch.BOOL)) else: # For non-equality logical ops (less/greater than, etc.), need to check both sides @@ -983,6 +985,7 @@ def compare_tagged_condition(self, self.add(Branch(check_lhs, int_block, rhs_block, Branch.BOOL)) self.activate_block(rhs_block) check_rhs = self.check_tagged_short_int(rhs, line, negated=True) + self.flush_keep_alives() self.add(Branch(check_rhs, int_block, short_int_block, Branch.BOOL)) # Arbitrary integers (slow path) self.activate_block(int_block) @@ -994,6 +997,7 @@ def compare_tagged_condition(self, if negate_result: self.add(Branch(call, false, true, Branch.BOOL)) else: + self.flush_keep_alives() self.add(Branch(call, true, false, Branch.BOOL)) # Short integers (fast path) self.activate_block(short_int_block) diff --git a/mypyc/test-data/irbuild-classes.test b/mypyc/test-data/irbuild-classes.test index 9fe5aba3bc60..5a574ac44354 100644 --- a/mypyc/test-data/irbuild-classes.test +++ b/mypyc/test-data/irbuild-classes.test @@ -1289,3 +1289,47 @@ L0: r1 = r0.e r2 = r1.x return r2 + +[case testBorrowResultOfCustomGetItemInIfStatement] +from typing import List + +class C: + def __getitem__(self, x: int) -> List[int]: + return [] + +def f(x: C) -> None: + # In this case the keep_alive must come before the branch, as otherwise + # reference count transform will get confused. + if x[1][0] == 2: + y = 1 + else: + y = 2 +[out] +def C.__getitem__(self, x): + self :: __main__.C + x :: int + r0 :: list +L0: + r0 = PyList_New(0) + return r0 +def f(x): + x :: __main__.C + r0 :: list + r1 :: object + r2 :: int + r3 :: bit + y :: int +L0: + r0 = x.__getitem__(2) + r1 = CPyList_GetItemShortBorrow(r0, 0) + r2 = unbox(int, r1) + r3 = r2 == 4 + keep_alive r0 + if r3 goto L1 else goto L2 :: bool +L1: + y = 2 + goto L3 +L2: + y = 4 +L3: + return 1 From 4ec79ba89aefd99094618c8aa1b86b369f84854d Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Thu, 19 May 2022 22:05:31 +0100 Subject: [PATCH 09/10] Fix tests --- mypyc/test-data/irbuild-generics.test | 1 + mypyc/test-data/irbuild-lists.test | 1 + 2 files changed, 2 insertions(+) diff --git a/mypyc/test-data/irbuild-generics.test b/mypyc/test-data/irbuild-generics.test index 64a03c9444e2..fe4a94992717 100644 --- a/mypyc/test-data/irbuild-generics.test +++ b/mypyc/test-data/irbuild-generics.test @@ -65,6 +65,7 @@ L0: r3 = borrow c.x r4 = unbox(int, r3) r5 = CPyTagged_Add(4, r4) + keep_alive c return 1 [case testGenericMethod] diff --git a/mypyc/test-data/irbuild-lists.test b/mypyc/test-data/irbuild-lists.test index 4a906089f68a..47f7ada709e3 100644 --- a/mypyc/test-data/irbuild-lists.test +++ b/mypyc/test-data/irbuild-lists.test @@ -42,6 +42,7 @@ L0: r1 = borrow cast(list, r0) r2 = CPyList_GetItemShort(r1, 2) r3 = unbox(int, r2) + keep_alive x, r0 return r3 [case testListSet] From 1afbb6c5aee11a6435958ae2f3c3188427ce43ce Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Sun, 22 May 2022 09:43:24 +0100 Subject: [PATCH 10/10] Fix typo --- mypyc/irbuild/ast_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypyc/irbuild/ast_helpers.py b/mypyc/irbuild/ast_helpers.py index 15d868f6552d..8c9ca186e46a 100644 --- a/mypyc/irbuild/ast_helpers.py +++ b/mypyc/irbuild/ast_helpers.py @@ -79,7 +79,7 @@ def is_borrow_friendly_expr(self: IRBuilder, expr: Expression) -> bool: Borrowing means keeping a reference without incrementing the reference count. """ if isinstance(expr, (IntExpr, FloatExpr, StrExpr, BytesExpr)): - # Literals are immportal and can always be borrowed + # Literals are immortal and can always be borrowed return True if (isinstance(expr, (UnaryExpr, OpExpr, NameExpr, MemberExpr)) and constant_fold_expr(self, expr) is not None):