Skip to content
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

Fix inliners to run all passes on IR and clean up correctly. #5673

Merged
merged 4 commits into from May 29, 2020

Conversation

stuartarchibald
Copy link
Contributor

As title.

Fixes #4691
Fixes #4693
Fixes #5476

Comment on lines +547 to +548
iinfo = _inline_info(ir, typemap, calltypes, sig)
self._inline_overloads[sig.args] = {'folded_args': folded_args,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +499 to +501
# Disable SSA transformation, the call site won't be in SSA form and
# self.inline_ir depends on this being the case.
state.flags.enable_ssa = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this true for overload inlines?

Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without it, this:

======================================================================
ERROR: test_inlining_optional_constant (numba.tests.test_ir_inlining.TestGeneralInlining)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "<path>numba/tests/test_ir_inlining.py", line 1043, in test_inlining_optional_constant
    self.check(impl, block_count='SKIP', inline_expect={'bar': True})
  File "<path>numba/tests/test_ir_inlining.py", line 103, in check
    self.assertEqual(test_impl(*args), j_func(*args))
  File "<path>numba/core/dispatcher.py", line 401, in _compile_for_args
    error_rewrite(e, 'typing')
  File "<path>numba/core/dispatcher.py", line 344, in error_rewrite
    reraise(type(e), e, None)
  File "<path>numba/core/utils.py", line 79, in reraise
    raise value.with_traceback(tb)
numba.core.errors.TypingError: Failed in inliner_custom_pipe mode pipeline (step: nopython frontend)
Type of variable 'b.1.382' cannot be determined, operation: unknown operation, location: unknown location (0:0)

File "unknown location", line 0:
<source missing, REPL/exec in use?>

Comment on lines +507 to +510
# Updating these causes problems?!
#fcomp.targetdescr.options.parse_as_flags(flags,
# fcomp.targetoptions)
#flags = fcomp._customize_flags(flags)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a problem? Do flags influence IR gen?

Copy link
Member

Choose a reason for hiding this comment

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

only in cfuncs it seems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leave it for now?

@sklam sklam self-requested a review May 5, 2020 15:52
stuartarchibald added a commit to stuartarchibald/numba that referenced this pull request May 5, 2020
@stuartarchibald
Copy link
Contributor Author

stuartarchibald@94f6bb2 achieves the same/similar, but as a strategy pushes inlining as high up the compilation chain as possible, and then just lets other passes run as usual. The inlined IR is not subjected to the full compilation pipelines, only closure inlining and generic rewrites.

@sklam
Copy link
Member

sklam commented May 7, 2020

Note: failing at

def test_inlined_unroll_list(self):
@njit(inline='always')
def foo(y):
x = [10, y]
r = 0
for a in literal_unroll(x):
r += a
return r
@njit
def bar():
return foo(12)
self.assertEqual(bar(), 10 + 12)
because of literal_unroll(x) where x = [10, y]

@sklam
Copy link
Member

sklam commented May 7, 2020

NOTE: prefer this PR over stuartarchibald@94f6bb2

@stuartarchibald
Copy link
Contributor Author

stuartarchibald commented May 13, 2020

Note: failing at

def test_inlined_unroll_list(self):
@njit(inline='always')
def foo(y):
x = [10, y]
r = 0
for a in literal_unroll(x):
r += a
return r
@njit
def bar():
return foo(12)
self.assertEqual(bar(), 10 + 12)

because of literal_unroll(x) where x = [10, y]

elect to skip for now, needs updates elsewhere

@sklam
Copy link
Member

sklam commented May 18, 2020

is this ready for review?

@stuartarchibald
Copy link
Contributor Author

is this ready for review?

Let's say yes to get it moving, but also note there's a couple of thing we may need to look at already mentioned in the above.

@stuartarchibald stuartarchibald marked this pull request as ready for review May 19, 2020 10:16
return self.inline_ir(caller_ir, block, i, callee_ir, freevars,
arg_typs=arg_typs)

def inline_closure(self, caller_ir, block, i, closure_obj, glbls,
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this function is dead

self.typemap.update(f_typemap)
self.calltypes.update(f_calltypes)

def convert_code_object_to_function(self, obj, glbls):
Copy link
Member

Choose a reason for hiding this comment

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

this too

@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels May 21, 2020
@stuartarchibald
Copy link
Contributor Author

I added some tests for the dead functions to the tip of this branch: https://github.com/stuartarchibald/numba/commits/fix/5476_5_cont

My conclusion from adding them is that the interface still isn't quite right and that it'd make sense to also add to the class something that actually does the inlining work (stitches the blocks together etc), as well as computing the changes required. I'm going to remove those dead methods from the class as a result and open a ticket about future things to be done.

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels May 29, 2020
@stuartarchibald
Copy link
Contributor Author

Work ticket: #5775

Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

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

Thanks for the fix here. The inliner passes are complicated and there may need further work to clean it up, but this patch already provides important fixes and we know some downstream projects will need.

@sklam sklam removed the 4 - Waiting on reviewer Waiting for reviewer to respond to author label May 29, 2020
@sklam sklam added the 5 - Ready to merge Review and testing done, is ready to merge label May 29, 2020
@sklam sklam merged commit f5b826c into numba:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
2 participants