Skip to content

Conversation

kaushikcfd
Copy link
Collaborator

Close #648

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks! Some thoughts below, mostly conceptual/documentation. Good to go once those are adressed. Mostly rename/philosophy/documentation.

skai = get_subkernel_arg_info(kernel, subkernel_name)
passed_names = skai.passed_names
passed_names = (skai.passed_names
if self.target.is_executable
Copy link
Owner

Choose a reason for hiding this comment

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

I feel that the real issue is a bit different from what's being handled here:

For GPU-ish targets, a kernel is a collection of "subkernel" enqueues. The interface between the "entrypoint/invoker" and the "subkernels" is internal. Meanwhile, for the C target as used by Firedrake, this distinction is not fully realized at the moment: Users will typically directly call the subkernels, and the "entrypoint/invoker" level just doesn't exist. That makes sense, because there's (always?) only one subkernel, and the invoker, if it existed, would just pass through arguments to the subkernel and call it a day. The flag you're introducing here effectively codifies the ability to skip the "entrypoint/invoker" level.

Could you also add some semblance of this discussion to the docs of single_subkernel_is_entrypoint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some docs on these lines in 9bfef04.

@sv2518
Copy link
Contributor

sv2518 commented Nov 8, 2022

I addressed the code review in https://github.com/inducer/loopy/pull/701/files, could you please re-review @inducer?

@kaushikcfd kaushikcfd requested a review from inducer November 10, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dropping arguments in inlining

3 participants