Skip to content

Fix rewrite pass affecting objmode lowering during fallback #2185

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

Merged
merged 3 commits into from
Nov 29, 2016

Conversation

sklam
Copy link
Member

@sklam sklam commented Nov 1, 2016

Rewrite passes mutates the IR in-place but the inserted opcode is not understood by the objmode lowering. When a function fails typing and fallbacks to the objmode, the IR is already mutated. This patch fixes it by preserving the original IR and restoring the IR upon entry to the objmode pipeline.

Closes #2159 and #2169

sklam added 3 commits October 31, 2016 18:02
Preserve original IR and restore if fallback to object mode.
Verify that rewrite pass doesn't affect objmode lowering

return len(getitems) > 0

def apply(self):
"""
Rewrite all matching getitems as static_getitems.
"""
for expr, const in self.getitems:
Copy link
Member Author

Choose a reason for hiding this comment

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

This pass introduce in opcode to the source IR. It is rewritten to create new IR node instead. We should consider refactoring all rewrite passes to avoid mutating the source IR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, the original spelling is much simpler, though. Perhaps Rewrite.match() should always be given a block copy? That way, the rewrite can mutate the IR if it likes to...

Copy link
Contributor

Choose a reason for hiding this comment

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

And / or add some methods to numba.ir to make copying instructions easier...

Copy link
Member Author

Choose a reason for hiding this comment

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

I am also working on a refactor on the rewrite passes to make it easier to write and also fix other IR mutating pass.

@sklam sklam added this to the Numba 0.30 milestone Nov 1, 2016
@codecov-io
Copy link

Current coverage is 87.03% (diff: 95.91%)

Merging #2185 into master will increase coverage by <.01%

@@             master      #2185   diff @@
==========================================
  Files           308        308          
  Lines         56735      56777    +42   
  Methods           0          0          
  Messages          0          0          
  Branches       5889       5893     +4   
==========================================
+ Hits          49377      49417    +40   
- Misses         6408       6409     +1   
- Partials        950        951     +1   

Powered by Codecov. Last update 9789438...189e598

@seibert
Copy link
Contributor

seibert commented Nov 1, 2016

@pitrou: Can you review this?

@pitrou
Copy link
Contributor

pitrou commented Nov 2, 2016

The general question is whether rewrite passes may also benefit object mode (in which case, an alternative solution is to support the new instructions in object mode too).

@sklam
Copy link
Member Author

sklam commented Nov 2, 2016

Some rewrite pass is useful for object mode but not all of them. Perhaps, objmode should get it's own set rewrite passes.

@sklam
Copy link
Member Author

sklam commented Nov 2, 2016

Also, I think the IR should be immutable after the Interpreter is done.

@seibert
Copy link
Contributor

seibert commented Nov 28, 2016

Any further review of this needed, @pitrou / @sklam?

@sklam
Copy link
Member Author

sklam commented Nov 29, 2016

This PR contains the fix. The refactor that I mentioned will be in a separate PR. The WIP branch is sklam/numba@fix/invalid_rewrite...sklam:refactor/rewrite_pass

@seibert seibert merged commit 01c8c0c into numba:master Nov 29, 2016
@sklam sklam deleted the fix/invalid_rewrite branch November 30, 2016 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.29.0 NotImplementedError, LoweringError with raise
4 participants