-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add dead branch prune before type inference. #3592
Merged
stuartarchibald
merged 6 commits into
numba:master
from
stuartarchibald:wip/dead_branch_prune
Feb 27, 2019
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
fbcf804
Add dead branch prune before type inference.
stuartarchibald 80428ac
Add tests for transforms.
stuartarchibald a5a3a6e
Fix flake8
stuartarchibald 2d2dc9e
Merge remote-tracking branch 'upstream/master' into wip/dead_branch_p…
stuartarchibald 55957f9
Fix up WRT feedback.
stuartarchibald 05ebb5e
Respond to feedback + flake8
stuartarchibald File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
|
||
from numba import ir | ||
from numba.controlflow import CFGraph | ||
from numba import types | ||
|
||
# | ||
# Analysis related to variable lifetime | ||
|
@@ -18,6 +19,7 @@ | |
# format: {type:function} | ||
ir_extension_usedefs = {} | ||
|
||
|
||
def compute_use_defs(blocks): | ||
""" | ||
Find variable use/def per block. | ||
|
@@ -259,3 +261,152 @@ def find_top_level_loops(cfg): | |
for loop in cfg.loops().values(): | ||
if loop.header not in blocks_in_loop: | ||
yield loop | ||
|
||
|
||
# Functions to manipulate IR | ||
def dead_branch_prune(func_ir, called_args): | ||
""" | ||
Removes dead branches based on constant inference from function args. | ||
This directly mutates the IR. | ||
|
||
func_ir is the IR | ||
called_args are the actual arguments with which the function is called | ||
""" | ||
from .ir_utils import get_definition, guard, find_const, GuardException | ||
|
||
DEBUG = 0 | ||
|
||
def find_branches(func_ir): | ||
# find *all* branches | ||
branches = [] | ||
for blk in func_ir.blocks.values(): | ||
branch_or_jump = blk.body[-1] | ||
if isinstance(branch_or_jump, ir.Branch): | ||
branch = branch_or_jump | ||
condition = guard(get_definition, func_ir, branch.cond.name) | ||
if condition is not None: | ||
branches.append((branch, condition, blk)) | ||
return branches | ||
|
||
def do_prune(take_truebr, blk): | ||
keep = branch.truebr if take_truebr else branch.falsebr | ||
# replace the branch with a direct jump | ||
jmp = ir.Jump(keep, loc=branch.loc) | ||
blk.body[-1] = jmp | ||
|
||
def prune_by_type(branch, condition, blk, *conds): | ||
# this prunes a given branch and fixes up the IR | ||
# at least one needs to be a NoneType | ||
lhs_cond, rhs_cond = conds | ||
lhs_none = isinstance(lhs_cond, types.NoneType) | ||
rhs_none = isinstance(rhs_cond, types.NoneType) | ||
if lhs_none or rhs_none: | ||
take_truebr = condition.fn(lhs_cond, rhs_cond) | ||
if DEBUG > 0: | ||
kill = branch.falsebr if take_truebr else branch.truebr | ||
print("Pruning %s" % kill, branch, lhs_cond, rhs_cond, | ||
condition.fn) | ||
do_prune(take_truebr, blk) | ||
return True | ||
return False | ||
|
||
def prune_by_value(branch, condition, blk, *conds): | ||
lhs_cond, rhs_cond = conds | ||
take_truebr = condition.fn(lhs_cond, rhs_cond) | ||
if DEBUG > 0: | ||
kill = branch.falsebr if take_truebr else branch.truebr | ||
print("Pruning %s" % kill, branch, lhs_cond, rhs_cond, condition.fn) | ||
do_prune(take_truebr, blk) | ||
return True | ||
|
||
class Unknown(object): | ||
pass | ||
|
||
def resolve_input_arg_const(input_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. ideally, this should be folded into |
||
""" | ||
Resolves an input arg to a constant (if possible) | ||
""" | ||
idx = func_ir.arg_names.index(input_arg) | ||
input_arg_ty = called_args[idx] | ||
|
||
# comparing to None? | ||
if isinstance(input_arg_ty, types.NoneType): | ||
return input_arg_ty | ||
|
||
# is it a kwarg default | ||
if isinstance(input_arg_ty, types.Omitted): | ||
val = input_arg_ty.value | ||
if isinstance(val, types.NoneType): | ||
return val | ||
elif val is None: | ||
return types.NoneType('none') | ||
|
||
# literal type, return the type itself so comparisons like `x == None` | ||
# still work as e.g. x = types.int64 will never be None/NoneType so | ||
# the branch can still be pruned | ||
return getattr(input_arg_ty, 'literal_type', Unknown()) | ||
|
||
if DEBUG > 1: | ||
print("before".center(80, '-')) | ||
print(func_ir.dump()) | ||
|
||
# This looks for branches where: | ||
# at least one arg of the condition is in input args and const | ||
# at least one an arg of the condition is a const | ||
# if the condition is met it will replace the branch with a jump | ||
branch_info = find_branches(func_ir) | ||
nullified_conditions = [] # stores conditions that have no impact post prune | ||
for branch, condition, blk in branch_info: | ||
const_conds = [] | ||
if isinstance(condition, ir.Expr) and condition.op == 'binop': | ||
prune = prune_by_value | ||
for arg in [condition.lhs, condition.rhs]: | ||
resolved_const = Unknown() | ||
if arg.name in func_ir.arg_names: | ||
# it's an e.g. literal argument to the function | ||
resolved_const = resolve_input_arg_const(arg.name) | ||
prune = prune_by_type | ||
else: | ||
# it's some const argument to the function, cannot use guard | ||
# here as the const itself may be None | ||
try: | ||
resolved_const = find_const(func_ir, arg) | ||
if resolved_const is None: | ||
resolved_const = types.NoneType('none') | ||
except GuardException: | ||
pass | ||
|
||
if not isinstance(resolved_const, Unknown): | ||
const_conds.append(resolved_const) | ||
|
||
# lhs/rhs are consts | ||
if len(const_conds) == 2: | ||
# prune the branch, switch the branch for an unconditional jump | ||
if(prune(branch, condition, blk, *const_conds)): | ||
# add the condition to the list of nullified conditions | ||
nullified_conditions.append(condition) | ||
|
||
# 'ERE BE DRAGONS... | ||
# It is the evaluation of the condition expression that often trips up type | ||
# inference, so ideally it would be removed as it is effectively rendered | ||
# dead by the unconditional jump if a branch was pruned. However, there may | ||
# be references to the condition that exist in multiple places (e.g. dels) | ||
# and we cannot run DCE here as typing has not taken place to give enough | ||
# information to run DCE safely. Upshot of all this is the condition gets | ||
# rewritten below into a benign const that typing will be happy with and DCE | ||
# can remove it and its reference post typing when it is safe to do so | ||
# (if desired). | ||
for _, condition, blk in branch_info: | ||
if condition in nullified_conditions: | ||
for x in blk.body: | ||
if isinstance(x, ir.Assign) and x.value is condition: | ||
x.value = ir.Const(0, loc=x.loc) | ||
|
||
# Remove dead blocks, this is safe as it relies on the CFG only. | ||
cfg = compute_cfg_from_blocks(func_ir.blocks) | ||
for dead in cfg.dead_nodes(): | ||
del func_ir.blocks[dead] | ||
|
||
if DEBUG > 1: | ||
print("after".center(80, '-')) | ||
print(func_ir.dump()) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
array analysis does some pruning for some reason: https://github.com/numba/numba/blob/master/numba/array_analysis.py#L1066
Maybe it is unnecessary now?
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.
Would have to check, array analysis isn't on the default code path though? Also I think that code is just looking at e.g.
if 1:
orif True:
?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.
That's correct, it's not on the default path.
Seems like it, but I don't remember the reason (upstream optimization produces
if True:
?). Just something to keep in mind for future, not necessarily this PR.