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.

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

Jump to bottom

add emit_excinfo_store_with_md for all excinfo-related store instructions #9551

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add emit_excinfo_store_with_md for all excinfo-related store instructions #9551

wants to merge 2 commits into from

Conversation

dlee992
Copy link
Contributor

@dlee992 dlee992 commented Apr 29, 2024 •
edited

fixes #9550

This bug is introduced between numba upgraded to llvm14 (#8535) and added the dynamic exception info feature (#8134).

@dlee992
Copy link
Contributor Author

dlee992 commented Apr 29, 2024

BTW, can I be upgraded to a collaborator? or can I be given the permission about marking issues and PRs? I can only use this permission to mark the issues and PRs opened by myself.

Ha, just asking...

@dlee992
Copy link
Contributor Author

dlee992 commented Apr 29, 2024 •
edited

Oh, if possible, I think this fix should be put in release ASAP, perhaps 0.60?
For my use-cases (heavily using various numba containers and structref), my performance could be hurt by 2~3x times, since un-removed refcount pairs.

The CI failures are network issues during conda install.

@esc
Copy link
Member

esc commented Apr 30, 2024

Oh, if possible, I think this fix should be put in release ASAP, perhaps 0.60? For my use-cases (heavily using various numba containers and structref), my performance could be hurt by 2~3x times, since un-removed refcount pairs.

This release is closed now, unfortunately. I've restarted CI and marked this as needing a review.

@esc
Copy link
Member

esc commented Apr 30, 2024

BTW, can I be upgraded to a collaborator? or can I be given the permission about marking issues and PRs? I can only use this permission to mark the issues and PRs opened by myself.

Ha, just asking..

Would suggest to join a dev meeting or office hour and ask about this.

@dlee992 dlee992 mentioned this pull request Apr 30, 2024
Open
guilhermeleobas
guilhermeleobas requested changes Apr 30, 2024
View reviewed changes
Copy link
Collaborator

@guilhermeleobas guilhermeleobas left a comment

Choose a reason for hiding this comment

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

@dlee992, thanks for the pull request. I gave an initial review and would be good if you could provide a bit more of context on how you're testing this PR.

numba/core/callconv.py
Comment on lines +397 to +401
def emit_excinfo_store_with_md(self, builder, value, excinfo_ptr):
store = builder.store(value, excinfo_ptr)
md = builder.module.add_metadata([ir.IntType(1)(1)])
store.set_metadata("numba_exception_output", md)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change this function to only emit the metadata and keep the store out?

Suggested change
def emit_excinfo_store_with_md(self, builder, value, excinfo_ptr):
store = builder.store(value, excinfo_ptr)
md = builder.module.add_metadata([ir.IntType(1)(1)])
store.set_metadata("numba_exception_output", md)
def emit_numba_exception_output_metadata(self, builder, store):
md = builder.module.add_metadata([ir.IntType(1)(1)])
store.set_metadata("numba_exception_output", md)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sure, but why we need to keep store out?

numba/core/callconv.py
store = builder.store(struct_gv, excptr)
md = builder.module.add_metadata([ir.IntType(1)(1)])
store.set_metadata("numba_exception_output", md)
self.emit_excinfo_store_with_md(builder, struct_gv, excptr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.emit_excinfo_store_with_md(builder, struct_gv, excptr)
store = builder.store(struct_gv, excptr)
self.emit_numba_exception_output_metadata(builder, store)

numba/core/callconv.py
@@ -712,7 +715,7 @@ def set_dynamic_user_exc(self, builder, exc, exc_args, nb_types, loc=None,
int32_t(len(struct_type)))
for idx, arg in enumerate(exc_fields):
builder.store(arg, builder.gep(excinfo_p, [zero, int32_t(idx)]))
builder.store(excinfo_p, excinfo_pp)
self.emit_excinfo_store_with_md(builder, excinfo_p, excinfo_pp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.emit_excinfo_store_with_md(builder, excinfo_p, excinfo_pp)
store = builder.store(excinfo_p, excinfo_pp)
self.emit_numba_exception_output_metadata(builder, store)

numba/core/callconv.py
@@ -762,12 +765,12 @@ def unset_try_status(self, builder):
# will run normally.
excinfoptr = self._get_excinfo_argument(builder.function)
null = cgutils.get_null_value(excinfoptr.type.pointee)
builder.store(null, excinfoptr)
self.emit_excinfo_store_with_md(builder, null, excinfoptr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.emit_excinfo_store_with_md(builder, null, excinfoptr)
store = builder.store(null, excinfoptr)
self.emit_numba_exception_output_metadata(builder, store)

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I wonder if this should be considered a raising block or not, as it is not raising an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! But I didn't know the answer. I just did this conservatively.

numba/core/callconv.py

def return_status_propagate(self, builder, status):
trystatus = self.check_try_status(builder)
excptr = self._get_excinfo_argument(builder.function)
builder.store(status.excinfoptr, excptr)
self.emit_excinfo_store_with_md(builder, status.excinfoptr, excptr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.emit_excinfo_store_with_md(builder, status.excinfoptr, excptr)
store = builder.store(status.excinfoptr, excptr)
self.emit_numba_exception_output_metadata(builder, store)

@dlee992
Copy link
Contributor Author

dlee992 commented Apr 30, 2024 •
edited

Would suggest to join a dev meeting or office hour and ask about this.

Sure, will try.

For the test case:
I think if the jitted code is related to structref or container types, then the code logic can touch the dynamic exception info generation, then we can find a simple test case for it. Need to make up one.

@guilhermeleobas guilhermeleobas added this to the 0.61.0-rc1 milestone May 1, 2024
@guilhermeleobas guilhermeleobas added the 4 - Waiting on author Waiting for author to respond to review label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guilhermeleobas guilhermeleobas guilhermeleobas requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
3 - Ready for Review 4 - Waiting on author Waiting for author to respond to review
Projects
None yet
Milestone
0.61.0-rc1
Development

Successfully merging this pull request may close these issues.

LLVM IR generation bug when adding metadata for excinfo-related store instruction
3 participants
@dlee992 @esc @guilhermeleobas