Skip to content

Jump-To-Jump elimination could be more aggressive. #93223

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

Closed
sweeneyde opened this issue May 25, 2022 · 4 comments
Closed

Jump-To-Jump elimination could be more aggressive. #93223

sweeneyde opened this issue May 25, 2022 · 4 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@sweeneyde
Copy link
Member

Example:

def f():
    for i in range(5):
        if i > 3:
            print(i)
  1           0 RESUME                   0

  2           2 LOAD_GLOBAL              1 (NULL + range)
             14 LOAD_CONST               1 (5)
             16 CALL                     1
             26 GET_ITER
        >>   28 FOR_ITER                21 (to 72)
             30 STORE_FAST               0 (i)

  3          32 LOAD_FAST                0 (i)
             34 LOAD_CONST               2 (3)
             36 COMPARE_OP               4 (>)
             42 POP_JUMP_FORWARD_IF_FALSE    13 (to 70)     <--------- could be BACKWARD to 28

  4          44 LOAD_GLOBAL              3 (NULL + print)
             56 LOAD_FAST                0 (i)
             58 CALL                     1
             68 POP_TOP
        >>   70 JUMP_BACKWARD           22 (to 28)

  2     >>   72 LOAD_CONST               0 (None)
             74 RETURN_VALUE

Something like this should suffice:

 static bool
 jump_thread(struct instr *inst, struct instr *target, int opcode)
 {
     assert(!IS_VIRTUAL_OPCODE(opcode) || IS_VIRTUAL_JUMP_OPCODE(opcode));
     assert(is_jump(inst));
     assert(is_jump(target));
     // bpo-45773: If inst->i_target == target->i_target, then nothing actually
     // changes (and we fall into an infinite loop):
-    if (inst->i_lineno == target->i_lineno &&
+    if ((inst->i_lineno == target->i_lineno || target->i_lineno == -1) &&
         inst->i_target != target->i_target)
     {
         inst->i_target = target->i_target;
         inst->i_opcode = opcode;
         return true;
     }
     return false;
 }

cc @markshannon @iritkatriel @brandtbucher

@sweeneyde sweeneyde added type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) and removed type-bug An unexpected behavior, bug, or error labels May 25, 2022
@brandtbucher
Copy link
Member

brandtbucher commented May 25, 2022

I tried something like this a few months ago, but (if I remember correctly) it ended up duplicating a few blocks in test_dis.py. The net effect was more code, not less, and it ended up actually having the same number of jumps in the final output for all code paths (because some paths had to jump over the new blocks now, I think).

I abandoned that experiment because I couldn’t determine the cause of the duplication. I wonder if that’s still the case (or if @iritkatriel knows what the problem might be).

@brandtbucher
Copy link
Member

(At any rate, I think my experience highlights just how opaque and complex the interactions between the different compiler passes are.)

@sweeneyde
Copy link
Member Author

Maybe something has changed between then and now, but that change now makes jumpy one instruction shorter and eliminates two jump-to-jumps.

#93229

@sweeneyde
Copy link
Member Author

These are apparently the sites in pyperformance that this applies to:

bm_chaos: Spline.GetIndex
bm_chaos: Spline.GetIndex
bm_deltablue: Planner.extract_plan_from_constraints
bm_deltablue: Planner.extract_plan_from_constraints
bm_deltablue: Planner.remove_propagate_from
bm_deltablue: Planner.remove_propagate_from
bm_deltablue: Planner.remove_propagate_from
bm_deltablue: Planner.add_constraints_consuming_to
bm_deltablue: Planner.add_constraints_consuming_to
bm_deltablue: chain_test
bm_deltablue: projection_test
bm_deltablue: projection_test
bm_go: Square.set_neighbours
bm_go: Square.set_neighbours
bm_go: Square.move
bm_go: Square.remove
bm_go: Square.remove
bm_go: Square.remove
bm_go: Board.useful_fast
bm_go: Board.useful
bm_go: Board.score
bm_go: Board.score
bm_go: Board.score
bm_go: Board.check
bm_go: Board.check
bm_go: Board.check
bm_go: Board.check
bm_go: Board.check
bm_go: Board.check
bm_go: UCTNode.update_path
bm_go: UCTNode.best_child
bm_go: UCTNode.best_child
bm_go: UCTNode.best_visited
bm_go: UCTNode.best_visited
bm_hexiom: Done.remove_unfixed
bm_hexiom: Done.remove_unfixed
bm_hexiom: Done.filter_tiles
bm_hexiom: Done.next_cell_min_choice
bm_hexiom: Done.next_cell_max_choice
bm_hexiom: Done.next_cell_highest_value
bm_hexiom: Done.next_cell_highest_value
bm_hexiom: Done.next_cell_first
bm_hexiom: Done.next_cell_max_neighbors
bm_hexiom: Done.next_cell_max_neighbors
bm_hexiom: Done.next_cell_min_neighbors
bm_hexiom: Done.next_cell_min_neighbors
bm_hexiom: Hex.link_nodes
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: constraint_pass
bm_hexiom: solved
bm_hexiom: solved
bm_hexiom: solved
bm_hexiom: solved
bm_hexiom: solved
bm_hexiom: solved
bm_hexiom: solve_step
bm_hexiom: solve_step
bm_hexiom: read_file
bm_hexiom: read_file
bm_json_dumps: main
bm_json_dumps: main
bm_json_loads: mutate_dict
bm_mdp: topoSort
bm_mdp: Battle.evaluate
bm_meteor_contest: permute
bm_meteor_contest: get_footprints
bm_meteor_contest: solve
bm_meteor_contest: solve
bm_meteor_contest: solve
bm_meteor_contest: solve
bm_meteor_contest: solve
bm_nqueens: n_queens
bm_pathlib: setup
bm_pickle: mutate_dict
bm_pickle/run_benchmark.py: qualname=<nil>
bm_pyflate: HuffmanTable.__init__
bm_pyflate: HuffmanTable.min_max_bits
bm_pyflate: HuffmanTable._find_symbol
bm_pyflate: HuffmanTable._find_symbol
bm_pyflate: HuffmanTable.find_next_symbol
bm_pyflate: HuffmanTable.find_next_symbol
bm_pyflate: HuffmanTable.find_next_symbol
bm_pyflate: compute_used
bm_pyflate: gzip_main
bm_pyflate: gzip_main
bm_raytrace: firstIntersection
bm_raytrace: firstIntersection
bm_raytrace: firstIntersection
bm_raytrace: Scene._lightIsVisible
bm_raytrace: Scene._lightIsVisible
bm_raytrace: Scene.visibleLights
bm_raytrace: SimpleSurface.colourAt
bm_scimark: MonteCarlo
bm_scimark: LU_factor
bm_scimark: LU_factor
bm_sqlite_synth: bench_sqlite
bm_tornado_http: bench_tornado.<locals>.run_client
bm_xml_etree: process
bm_xml_etree: bench_generate

As a more concrete example, this is the difference in bm_hexiom dis outputs before and after the change. 28 occurrences of >>\s+\d+\s+JUMP goes down to just 6, and all of these remaining 6 are FOR_ITER --> JUMP_BACKWARD, which we can't fold for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants