Skip to content

Initial refactoring of parfor reduction lowering - #7837

Merged
sklam merged 3 commits into
numba:mainfrom
sklam:misc/parforlowering
Mar 1, 2022
Merged

sklam merged 3 commits into
numba:mainfrom
sklam:misc/parforlowering

Conversation

@sklam

@sklam sklam commented Feb 11, 2022

Copy link
Copy Markdown
Member

Summary:

  • Breakup long functions to highlight the logic structure
  • Replace manually constructed Numba IR with jitted python code.

In the long run, we want to move parfor lowering outside of the LoweringPass.

- Breakup long functions to highlight the logic structure
- Replace manually constructed Numba IR with jitted python code.

@sklam sklam left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Notes for reviewers

Comment on lines +332 to +340
# Cleanup reduction variable
for v in redarrs.values():
lowerer.lower_inst(ir.Del(v.name, loc=loc))
# Restore the original typemap of the function that was replaced temporarily at the
# Beginning of this function.
lowerer.fndesc.typemap = orig_typemap

if config.DEBUG_ARRAY_OPT_RUNTIME:
res_print_str = "res_print"
strconsttyp = types.StringLiteral(res_print_str)
if config.DEBUG_ARRAY_OPT:
print("_lower_parfor_parallel done")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These are unchanged

Comment on lines +328 to +330
_parfor_lowering_finalize_reduction(
parfor, redarrs, lowerer, parfor_reddict,
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Majority of changes starts with this new function

@sklam
sklam marked this pull request as ready for review February 17, 2022 20:07
@stuartarchibald

Copy link
Copy Markdown
Contributor

@DrTodd13 please could you take an initial look at this as codeowner? Many thanks! I'm very much in favour of these changes in principle as it reduces reliance on raw IR generation.

@stuartarchibald stuartarchibald added the Effort - long Long size effort needed label Feb 18, 2022
DrTodd13
DrTodd13 previously approved these changes Feb 18, 2022

@DrTodd13 DrTodd13 left a comment

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.

I see where you are going with these changes and I appreciate the work. As most of this is conceptually just a code reorg I think it should be fine. If there are any errors introduced in the process that are subtle then I don't think humans (or at least me) are capable of spotting those problems in the code review. As long as all the tests pass then I'm happy with these changes.

@stuartarchibald stuartarchibald left a comment

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 refactoring this @sklam, I think it's easier to follow with less reliance on code gen. I've taken a look at the changes and it seems like the new code does the equivalent of what was there previously. There's a couple of comments inline to take a look at, but as @DrTodd13 suggests, the test suite is probably a better at catching mistakes than humans trying to translate between the two styles of code generation. Thanks again!

Comment thread numba/parfors/parfor_lowering.py Outdated
Comment thread numba/parfors/parfor_lowering.py
Comment thread numba/parfors/parfor_lowering.py
@stuartarchibald stuartarchibald added this to the Numba 0.56 RC milestone Feb 23, 2022
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Feb 23, 2022
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

@stuartarchibald stuartarchibald left a comment

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 patch and fixes.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Feb 28, 2022
@sklam
sklam merged commit 3b5395c into numba:main Mar 1, 2022
@sklam
sklam deleted the misc/parforlowering branch March 1, 2022 00:34
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 Effort - long Long size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants