-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] feat: specialize any and all using for loop helpers if possible [1/2]
#19948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 12 commits
41c7c9b
50bd3ce
f636c95
1af0a05
df669fe
2b43f62
e0dc704
1028f8c
70fb118
1e22947
6a2c8a0
a2ab6ba
f4c1c77
ded8359
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| RefExpr, | ||
| StrExpr, | ||
| SuperExpr, | ||
| SymbolNode, | ||
| TupleExpr, | ||
| Var, | ||
| ) | ||
|
|
@@ -80,6 +81,9 @@ | |
| from mypyc.irbuild.constant_fold import constant_fold_expr | ||
| from mypyc.irbuild.for_helpers import ( | ||
| comprehension_helper, | ||
| create_synthetic_nameexpr, | ||
| expr_has_specialized_for_helper, | ||
| for_loop_helper, | ||
| sequence_from_generator_preallocate_helper, | ||
| translate_list_comprehension, | ||
| translate_set_comprehension, | ||
|
|
@@ -412,29 +416,74 @@ def translate_safe_generator_call( | |
|
|
||
| @specialize_function("builtins.any") | ||
| def translate_any_call(builder: IRBuilder, expr: CallExpr, callee: RefExpr) -> Value | None: | ||
| if ( | ||
| len(expr.args) == 1 | ||
| and expr.arg_kinds == [ARG_POS] | ||
| and isinstance(expr.args[0], GeneratorExpr) | ||
| ): | ||
| return any_all_helper(builder, expr.args[0], builder.false, lambda x: x, builder.true) | ||
| if len(expr.args) == 1 and expr.arg_kinds == [ARG_POS]: | ||
| arg = expr.args[0] | ||
| if isinstance(arg, GeneratorExpr): | ||
| return any_all_helper(builder, arg, builder.false, lambda x: x, builder.true) | ||
| elif expr_has_specialized_for_helper(builder, arg): | ||
| retval = Register(bool_rprimitive) | ||
| builder.assign(retval, builder.false(), -1) | ||
| loop_exit = BasicBlock() | ||
| index_name = "__mypyc_any_item__" | ||
|
|
||
| def body_insts() -> None: | ||
| true_block = BasicBlock() | ||
| false_block = BasicBlock() | ||
| builder.add_bool_branch(builder.read(index_reg), true_block, false_block) | ||
| builder.activate_block(true_block) | ||
| builder.assign(retval, builder.true(), -1) | ||
| builder.goto(loop_exit) | ||
| builder.activate_block(false_block) | ||
|
|
||
| index_type = builder._analyze_iterable_item_type(arg) | ||
| index = create_synthetic_nameexpr(builder, index_name, index_type) | ||
| index_reg = builder.add_local_reg( | ||
| cast(SymbolNode, index.node), builder.type_to_rtype(index_type) | ||
| ) | ||
|
|
||
| for_loop_helper(builder, index, arg, body_insts, None, is_async=False, line=expr.line) | ||
| builder.goto_and_activate(loop_exit) | ||
| return retval | ||
| return None | ||
|
|
||
|
|
||
| @specialize_function("builtins.all") | ||
| def translate_all_call(builder: IRBuilder, expr: CallExpr, callee: RefExpr) -> Value | None: | ||
| if ( | ||
| len(expr.args) == 1 | ||
| and expr.arg_kinds == [ARG_POS] | ||
| and isinstance(expr.args[0], GeneratorExpr) | ||
| ): | ||
| return any_all_helper( | ||
| builder, | ||
| expr.args[0], | ||
| builder.true, | ||
| lambda x: builder.unary_op(x, "not", expr.line), | ||
| builder.false, | ||
| ) | ||
| if len(expr.args) == 1 and expr.arg_kinds == [ARG_POS]: | ||
| arg = expr.args[0] | ||
| if isinstance(arg, GeneratorExpr): | ||
| return any_all_helper( | ||
| builder, | ||
| arg, | ||
| builder.true, | ||
| lambda x: builder.unary_op(x, "not", expr.line), | ||
| builder.false, | ||
| ) | ||
|
|
||
| elif expr_has_specialized_for_helper(builder, arg): | ||
| retval = Register(bool_rprimitive) | ||
| builder.assign(retval, builder.true(), -1) | ||
| loop_exit = BasicBlock() | ||
| index_name = "__mypyc_all_item__" | ||
BobTheBuidler marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def body_insts() -> None: | ||
| true_block = BasicBlock() | ||
| false_block = BasicBlock() | ||
| builder.add_bool_branch(builder.read(index_reg), true_block, false_block) | ||
| builder.activate_block(false_block) | ||
| builder.assign(retval, builder.false(), -1) | ||
| builder.goto(loop_exit) | ||
| builder.activate_block(true_block) | ||
|
|
||
| index_type = builder._analyze_iterable_item_type(arg) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should I deduplicate this block or is this fine? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be useful to deduplicate, since the only difference seems to be true/false switch, so a helper would just need a bool flag that indicates whether it's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets just keep it as-is, the intent of the code is explicit and clear |
||
| index = create_synthetic_nameexpr(builder, index_name, index_type) | ||
| index_reg = builder.add_local_reg( | ||
| cast(SymbolNode, index.node), builder.type_to_rtype(index_type) | ||
| ) | ||
|
|
||
| for_loop_helper(builder, index, arg, body_insts, None, is_async=False, line=expr.line) | ||
| builder.goto_and_activate(loop_exit) | ||
| return retval | ||
| return None | ||
|
|
||
|
|
||
|
|
@@ -610,12 +659,9 @@ def translate_isinstance(builder: IRBuilder, expr: CallExpr, callee: RefExpr) -> | |
| return builder.builder.isinstance_helper(obj, irs, expr.line) | ||
|
|
||
| if isinstance(type_expr, RefExpr): | ||
| node = type_expr.node | ||
| if node: | ||
| desc = isinstance_primitives.get(node.fullname) | ||
| if desc: | ||
| obj = builder.accept(obj_expr) | ||
| return builder.primitive_op(desc, [obj], expr.line) | ||
| if node := type_expr.node: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was definitely not necessary but it bothered me how ugly that snippet was |
||
| if desc := isinstance_primitives.get(node.fullname): | ||
| return builder.primitive_op(desc, [builder.accept(obj_expr)], expr.line) | ||
|
|
||
| elif isinstance(type_expr, TupleExpr): | ||
| node_names: list[str] = [] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to understand why this check is useful. The idea is that if we'd have a generic for loop (
PyObject_GetIteretc.) we can as well call the stdlib function?What would happen if we'd always created the for loop here, since it would likely avoid the overhead of calling
anyusing generic call op, and the namespace dict lookup? If that sounds feasible, can you run some microbenchmarks to check if it's useful? I think it might be worth it especially if the iterable is small (0 or 1 items), which is quite common in practice.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"It took me a while to understand why this check is useful. The idea is that if we'd have a generic for loop (PyObject_GetIter etc.) we can as well call the stdlib function?"
Uh. No. While I appreciate your assessment, it wasn't exactly so thought-out. My only intent was to fall back to the existing logic, whatever that may be, if there is no supported ForHelper.
I need to think about how to implement this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait I want to confirm, you're saying if we just remove the if check, we're good to go without any other changes? I suppose that makes sense since the for_loop_helper does have the generic ForGenerator class but I wonder if we're missing any edge cases where that might cause failure