-
Notifications
You must be signed in to change notification settings - Fork 72
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
Respect kernel.arguments
in non-executable targets' lowered code
#650
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
loopy/target/c/__init__.py
Outdated
@@ -859,7 +859,9 @@ def get_function_declaration(self, codegen_state: CodeGenerationState, | |||
# subkernel launches occur only as part of entrypoint kernels for now | |||
from loopy.schedule.tools import get_subkernel_arg_info | |||
skai = get_subkernel_arg_info(kernel, subkernel_name) | |||
passed_names = skai.passed_names | |||
passed_names = (skai.passed_names | |||
if self.target.is_executable |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
I addressed the code review in https://github.com/inducer/loopy/pull/701/files, could you please re-review @inducer? |
…_entrypoint, add docs and error when there is more than one subkernel. Update how a subkernel is detected.
Close #648