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.

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

Add SYCL as a target #44

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

Conversation

NagithaAbeywickrema
Copy link

No description provided.

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 from a quick scroll.

@@ -0,0 +1,240 @@
__copyright__ = "Copyright (C) 2011-20 Andreas Kloeckner"
Copy link
Owner

Choose a reason for hiding this comment

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

Please fix the copyright statement.

)


def dtype_to_cltype(dtype):
Copy link
Owner

Choose a reason for hiding this comment

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

This looks like it's duplicated from CL. Please just import from there.

Comment on lines +236 to +238
yield "{}.submit({});".format(
self.queue, ", ".join(str(ad) for ad in self.args)
)
Copy link
Owner

Choose a reason for hiding this comment

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

Is this special syntax? This looks like a normal expression, and expressions are out of scope for cgen. (See, e.g. pymbolic.)

Copy link
Author

Choose a reason for hiding this comment

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

@inducer We have a small thing to clarify. You are asking to expose an API that would generate this expression step by step right? An example that would be generated by this would be,

queue_.submit([&](sycl::handler &handler) {
  handler.parallel_for(range_, [=](sycl::nd_item<3> item) {
    a[item.get_group(0)] = item.get_group(0);
  });
})

Let's assume we have a class cgen.Variable which constructs the queue_ part and the cgen.Call would call .submit on queue_ where cgen.Variable and cgen.Call are exposed as an API which helps generate the above expression.

Is this similar to the change that you expect?

Copy link
Owner

Choose a reason for hiding this comment

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

All of the things here are expressions (the call, the lambda, all of it). cgen doesn't currently model expressions, relying on pymbolic for that, though pymbolic better models Python than C++ ASTs. cgen is unlikely to become a full C++ AST, or I at least have no intention of taking it in that direction. Maybe you'll want to fork?

Copy link
Author

@NagithaAbeywickrema NagithaAbeywickrema Nov 21, 2023

Choose a reason for hiding this comment

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

Is it an acceptable change if we handle the expressions on the loopy side such that SYCL target will offer similar functionality as existing backends of cgen?

Comment on lines +221 to +223
yield "{}.parallel_for({});".format(
self.handler, ", ".join(str(ad) for ad in self.args)
)
Copy link
Owner

Choose a reason for hiding this comment

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

Is this special syntax? This looks like a normal expression, and expressions are out of scope for cgen. (See, e.g. pymbolic.)

while len(dim) < 3:
dim = dim + (1,)
self.dim = dim
self.decl = 'extern "C" [[sycl::reqd_work_group_size({})]]'.format(
Copy link
Owner

Choose a reason for hiding this comment

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

Is the extern "C" necessarily part of this? It would seem that this prohibits overloading.

mapper_method = "map_sycl_global"


# TODO sycl Image
Copy link
Owner

Choose a reason for hiding this comment

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

What remains to be implemented?

else:
return ["auto&"], Assign(
sub_decl,
f"*sycl::ext::oneapi::group_local_memory<{sub_tp[0]}>"
Copy link
Owner

Choose a reason for hiding this comment

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

Is local memory not exposed in sycl? (without using an extension)

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.

None yet

3 participants