Skip to content

Remove mk_unique_var in untyped_passes.py #8266

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.

Sign up for GitHub

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

Merged
sklam merged 6 commits into from
Aug 22, 2022
Merged

Remove mk_unique_var in untyped_passes.py #8266

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
06b9ef0
Remove `mk_unique_var` in `untyped_passes.py`
guilhermeleobas Jul 20, 2022
78f73df
Fix CI failure
guilhermeleobas Jul 25, 2022
5c627af
Fix another bug
guilhermeleobas Jul 26, 2022
ad2879a
another attempt to fix ci issue
guilhermeleobas Jul 26, 2022
a11fe01
flake8
guilhermeleobas Jul 26, 2022
c6a2506
replace scope.redefine by scope.define
guilhermeleobas Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions numba/core/untyped_passes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
compute_use_defs)
from numba.core.ir_utils import (guard, resolve_func_from_module, simplify_CFG,
GuardException, convert_code_obj_to_function,
mk_unique_var, build_definitions,
build_definitions,
replace_var_names, get_name_var_table,
compile_to_numba_ir, get_definition,
find_max_label, rename_labels,
Expand Down Expand Up @@ -866,7 +866,9 @@ def inject_loop_body(self, switch_ir, loop_ir, caller_max_label,
if (isinstance(stmt.value, ir.Expr) and
stmt.value.op == "typed_getitem"):
if isinstance(branch_ty, types.Literal):
new_const_name = mk_unique_var("branch_const")
scope = switch_ir.blocks[lbl].scope
new_const_name = scope.redefine(
"branch_const", stmt.loc).name
new_const_var = ir.Var(
blk.scope, new_const_name, stmt.loc)
new_const_val = ir.Const(
Expand Down Expand Up @@ -903,7 +905,15 @@ def inject_loop_body(self, switch_ir, loop_ir, caller_max_label,

new_var_dict = {}
for name, var in var_table.items():
new_var_dict[name] = mk_unique_var(name)
scope = switch_ir.blocks[lbl].scope
try:
scope.get_exact(name)
except errors.NotDefinedError:
# is this correct? In case the scope doesn't have the
# variable, we need to define it prior creating new
# copies of it!
scope.define(name, var.loc)
new_var_dict[name] = scope.redefine(name, var.loc).name

@guilhermeleobas guilhermeleobas Jul 26, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a careful look. Previously, mk_unique_var would return a unique identifier, whereas scope.redefine might just return the original identifier if it is the first time defining it.

@stuartarchibald stuartarchibald Aug 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilhermeleobas which tests are failing due to this?

@guilhermeleobas guilhermeleobas Aug 15, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this line to

            for name, var in var_table.items():
                scope = switch_ir.blocks[lbl].scope
                scope.define(name, var.loc)
                new_var_dict[name] = scope.redefine(name, var.loc).name

And run the tests python -m runtests numba.tests.test_mixed_tuple_unroller.TestMixedTupleUnroll.test_08

$ python -m runtests numba.tests.test_mixed_tuple_unroller.TestMixedTupleUnroll.test_08
/home/guilhermeleobas/git/numba/numba/core/utils.py:612: NumbaExperimentalFeatureWarning: First-class function type feature is experimental
  warnings.warn("First-class function type feature is experimental",
/home/guilhermeleobas/git/numba/numba/tests/test_mixed_tuple_unroller.py:603: NumbaExperimentalFeatureWarning: First-class function type feature is experimental
  acc += tup2[0]()
/home/guilhermeleobas/git/numba/numba/tests/test_mixed_tuple_unroller.py:605: NumbaExperimentalFeatureWarning: First-class function type feature is experimental
  acc += tup2[1]()
/home/guilhermeleobas/git/numba/numba/tests/test_mixed_tuple_unroller.py:607: NumbaExperimentalFeatureWarning: First-class function type feature is experimental
  acc += tup2[2]()
/home/guilhermeleobas/git/numba/numba/tests/test_mixed_tuple_unroller.py:603: NumbaExperimentalFeatureWarning: First-class function type feature is experimental
  acc += tup2[0]()
/home/guilhermeleobas/git/numba/numba/tests/test_mixed_tuple_unroller.py:605: NumbaExperimentalFeatureWarning: First-class function type feature is experimental
  acc += tup2[1]()
/home/guilhermeleobas/git/numba/numba/tests/test_mixed_tuple_unroller.py:607: NumbaExperimentalFeatureWarning: First-class function type feature is experimental
  acc += tup2[2]()
E
======================================================================
ERROR: test_08 (numba.tests.test_mixed_tuple_unroller.TestMixedTupleUnroll)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/guilhermeleobas/git/numba/numba/tests/test_mixed_tuple_unroller.py", line 617, in test_08
    self.assertEqual(foo(tup1, tup2), foo.py_func(tup1, tup2))
  File "/home/guilhermeleobas/git/numba/numba/core/dispatcher.py", line 476, in _compile_for_args
    error_rewrite(e, 'interpreter')
  File "/home/guilhermeleobas/git/numba/numba/core/dispatcher.py", line 407, in error_rewrite
    raise e
  File "/home/guilhermeleobas/git/numba/numba/core/dispatcher.py", line 420, in _compile_for_args
    return_val = self.compile(tuple(argtypes))
  File "/home/guilhermeleobas/git/numba/numba/core/dispatcher.py", line 965, in compile
    cres = self._compiler.compile(args, return_type)
  File "/home/guilhermeleobas/git/numba/numba/core/dispatcher.py", line 125, in compile
    status, retval = self._compile_cached(args, return_type)
  File "/home/guilhermeleobas/git/numba/numba/core/dispatcher.py", line 139, in _compile_cached
    retval = self._compile_core(args, return_type)
  File "/home/guilhermeleobas/git/numba/numba/core/dispatcher.py", line 152, in _compile_core
    cres = compiler.compile_extra(self.targetdescr.typing_context,
  File "/home/guilhermeleobas/git/numba/numba/core/compiler.py", line 716, in compile_extra
    return pipeline.compile_extra(func)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler.py", line 452, in compile_extra
    return self._compile_bytecode()
  File "/home/guilhermeleobas/git/numba/numba/core/compiler.py", line 520, in _compile_bytecode
    return self._compile_core()
  File "/home/guilhermeleobas/git/numba/numba/core/compiler.py", line 499, in _compile_core
    raise e
  File "/home/guilhermeleobas/git/numba/numba/core/compiler.py", line 486, in _compile_core
    pm.run(self.state)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 368, in run
    raise patched_exception
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 356, in run
    self._runPass(idx, pass_inst, state)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_lock.py", line 35, in _acquire_compile_lock
    return func(*args, **kwargs)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 311, in _runPass
    mutated |= check(pss.run_pass, internal_state)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 273, in check
    mangled = func(compiler_state)
  File "/home/guilhermeleobas/git/numba/numba/core/untyped_passes.py", line 1667, in run_pass
    pm.run(state)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 368, in run
    raise patched_exception
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 356, in run
    self._runPass(idx, pass_inst, state)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_lock.py", line 35, in _acquire_compile_lock
    return func(*args, **kwargs)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 311, in _runPass
    mutated |= check(pss.run_pass, internal_state)
  File "/home/guilhermeleobas/git/numba/numba/core/compiler_machinery.py", line 273, in check
    mangled = func(compiler_state)
  File "/home/guilhermeleobas/git/numba/numba/core/untyped_passes.py", line 1296, in run_pass
    stat = self.apply_transform(state)
  File "/home/guilhermeleobas/git/numba/numba/core/untyped_passes.py", line 1160, in apply_transform
    self.unroll_loop(state, info)
  File "/home/guilhermeleobas/git/numba/numba/core/untyped_passes.py", line 1259, in unroll_loop
    unrolled_body = self.inject_loop_body(
  File "/home/guilhermeleobas/git/numba/numba/core/untyped_passes.py", line 916, in inject_loop_body
    scope.define(name, var.loc)
  File "/home/guilhermeleobas/git/numba/numba/core/ir.py", line 1107, in define
    self.localvars.define(v.name, v)
  File "/home/guilhermeleobas/git/numba/numba/core/ir.py", line 261, in define
    raise RedefinedError(name)
numba.core.errors.RedefinedError: Failed in nopython mode pipeline (step: handles literal_unroll)
Failed in literal_unroll_subpipeline mode pipeline (step: performs mixed container unroll)
$const78.3

----------------------------------------------------------------------
Ran 1 test in 0.203s

FAILED (errors=1)
(numba)

@stuartarchibald stuartarchibald Aug 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the example, I think I understand what's happening. The switch_ir is some IR containing the switch table to jump to a type specific loop body version, it initially won't contain any of the variables present in the loop blocks scheduled for versioning. This part of the code is essentially wiring these versioned blocks into the switch table, which means the switch table need to "know" about the loops' variables as they are now in scope. Occasionally there are collisions, like in the case above, whereby the switch table happens to have the same name for a constant as a versioned loop body and the proposed change works around this.

I think another way to "fix" it would be to version the inlined variable names with the branch version from which they were derived, e.g.

diff --git a/numba/core/untyped_passes.py b/numba/core/untyped_passes.py
--- a/numba/core/untyped_passes.py
+++ b/numba/core/untyped_passes.py
@@ -906,14 +906,7 @@ class MixedContainerUnroller(FunctionPass):
             new_var_dict = {}
             for name, var in var_table.items():
                 scope = switch_ir.blocks[lbl].scope
-                try:
-                    scope.get_exact(name)
-                except errors.NotDefinedError:
-                    # is this correct? In case the scope doesn't have the
-                    # variable, we need to define it prior creating new
-                    # copies of it!
-                    scope.define(name, var.loc)
-                new_var_dict[name] = scope.redefine(name, var.loc).name
+                new_var_dict[name] = scope.define(f"v{branch_ty}_{name}", var.loc).name
             replace_var_names(loop_blocks, new_var_dict)

which might be a strategy worth employing as it makes it easier to track the origin of the variable.

Either way, I think the proposed code change is correct.

replace_var_names(loop_blocks, new_var_dict)

# clobber the sentinel body and then stuff in the rest
Expand Down Expand Up @@ -1358,14 +1368,11 @@ def transform(self, loop, func_ir, cfg):
def get_range(a):
return range(len(a))

def tokenise(x):
return mk_unique_var("CANONICALISER_%s" % x)

iternext = [_ for _ in
func_ir.blocks[loop.header].find_exprs('iternext')][0]
LOC = func_ir.blocks[loop.header].loc
get_range_var = ir.Var(func_ir.blocks[loop.header].scope,
tokenise('get_range_gbl'), LOC)
scope = func_ir.blocks[loop.header].scope
get_range_var = scope.redefine("CANONICALISER_get_range_gbl", LOC)
get_range_global = ir.Global('get_range', get_range, LOC)
assgn = ir.Assign(get_range_global, get_range_var, LOC)

Expand All @@ -1389,8 +1396,7 @@ def tokenise(x):
raise ValueError("problem")

# create a range(len(tup)) and inject it
call_get_range_var = ir.Var(entry_block.scope,
tokenise('call_get_range'), LOC)
call_get_range_var = scope.redefine('CANONICALISER_call_get_range', LOC)
make_call = ir.Expr.call(get_range_var, (stmt.value.value,), (), LOC)
assgn_call = ir.Assign(make_call, call_get_range_var, LOC)
entry_block.body.insert(idx, assgn_call)
Expand Down