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 code example and new slides #371

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

add code example and new slides #371

wants to merge 2 commits into from

Conversation

TApplencourt
Copy link
Collaborator

We have "hands-on" on the name, so lot of of example.
Copy of https://github.com/argonne-lcf/sycltrain/tree/master/9_sycl_of_hell

Will not go over nd_range as it covered later in the day.

Copy link
Collaborator

@hdelan hdelan left a comment

Choose a reason for hiding this comment

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

Looks good mostly, a few nits. In general the code could do with a clang-format using the config that was added in #368

Code_Exercises/9_sycl_of_hell/1_my_first_kernel.cpp Outdated Show resolved Hide resolved
// wait the queued submissions to complete
Q.wait();

return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: we don't need this since C++11

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh. Nice, forgot about it
IMO It's kind of ugly that an int function returns nothing... But I can remove it if you have a strong preference.

} catch (const std::runtime_error &err) {
std::cout << err.what() << std::endl;
std::cout << program;
exit(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

std::abort is maybe better

Copy link
Collaborator Author

@TApplencourt TApplencourt Nov 13, 2024

Choose a reason for hiding this comment

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

It print Aborted in my terminal pretty ugly... Can do std::exit(1) as a middle ground? :)

Code_Exercises/9_sycl_of_hell/3_nd_range.cpp Outdated Show resolved Hide resolved
Code_Exercises/9_sycl_of_hell/3_nd_range.cpp Show resolved Hide resolved
Code_Exercises/9_sycl_of_hell/3_nd_range.cpp Outdated Show resolved Hide resolved
@@ -0,0 +1,22 @@
CXX ?= icpx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need a Makefile as well as a CMakeLists.txt?

Copy link
Collaborator Author

@TApplencourt TApplencourt Nov 13, 2024

Choose a reason for hiding this comment

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

Because I hate CMakeList and have no clue how do debug it.
More seriously because this CMakeList will not work for adaptive CPP and like the simplificy of Makefile.

global_range,
[=](sycl::id<1> idx) {
// Explicit cast because of printf shenaningan.
sycl::ext::oneapi::experimental::printf("Hello, World! World rank %d\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what you've planned exactly here, but from the outside it doesn't really feel appropriate to me to use implementation-specific extensions in a generic SYCL tutorial.

Copy link
Collaborator Author

@TApplencourt TApplencourt Nov 13, 2024

Choose a reason for hiding this comment

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

Yeah, I agree... You support printf aren't you? We can IFDEF and write a "portable" printf and

Without the "print," I need to use the horrible sycl::stream, but then I need to teach command group handler, and it has always been a pain...

So I just took the "less horrible" thingy, but I don't like it either.

Copy link
Collaborator

@illuhad illuhad Nov 13, 2024

Choose a reason for hiding this comment

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

Ah, in the context of moving away from handler I can see the issue.

We don't universally have a variadic printf (and we most certainly will not implement oneAPI interfaces after Intel has set the precedent of implementing AdaptiveCpp extensions but changing the extension name without technical reason so that the API becomes incompatible).

But if it's acceptable to just print "Hello World" without rank, I can give you some internal detail function for printing at least strings, which would happen to work for now ;)
That would be sycl::detail::print(const char* s).

Copy link
Collaborator Author

@TApplencourt TApplencourt Nov 13, 2024

Choose a reason for hiding this comment

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

Yes, good enough! I can always cast my int into string for the

  Q.parallel_for(global_range, [=](sycl::id<1> idx) {
    // Explicit cast because of printf shenaningan.
    sycl::ext::oneapi::experimental::printf("Hello, World! World rank %d\n", static_cast<int>(idx));
  });

:)

Yeah, nested lambda tripped off a lot of people in the previous tutorial... Even more the Fortran dev who saw c++ for the "first" time. (but maybe I was just particularly bad at explaining cgh...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great :) Main reason there's no int supported directly is because I didn't want to spend a lot of time developing a generic device string formatting library :P But just for int it's reasonably simple.

Yeah, I'm all for killing buffer and command group handler ;)
These are just additional unnecessary complexities in my opinion.

# Polaris
module use /soft/modulefiles
module load oneapi/upstream
CXX=clang++ CXXFLAGS="-fsycl -fsycl-targets=nvptx64-nvidia-cuda -Xsycl-target-backend --cuda-gpu-arch=sm_80" make -j
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had set up an AdaptiveCpp installation on Polaris for the IWOCL tutorial. If there's interest I could see if that still works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes! With pleasure. Can update the cmake too if you want :P

Copy link
Collaborator

@illuhad illuhad Nov 13, 2024

Choose a reason for hiding this comment

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

Okay, will try Polaris tomorrow.

We always had working AdaptiveCpp in SYCLAcademy cmake. I think the only thing that would need to change in your CMakeLists.txt is a find_package(AdaptiveCpp).
Not sure if it's an issue if add_sycl_to_target is invoked on every source file (usually it's invoked once per target with a list of source files), but probably it will work.
AdaptiveCpp already ships this cmake interface.

The corresponding changes in the Makefile would be CXX=acpp and no -fsycl (empty CXXFLAGS, except standard arguments like -O3 etc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if it's an issue if add_sycl_to_target is invoked on every source file (usually it's invoked once per target with a list of source files), but probably it will work.

Each .cpp are independent mains, so can use the foreach hack :)

We always had working AdaptiveCpp in SYCLAcademy cmake. I think the only thing that would need to change in your CMakeLists.txt is a find_package(AdaptiveCpp).

Oh then perfect. Can try on Polaris with Adaptive cpp (when you give me the green light) and move to the SYCLAcamedy CMake away

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I missed that these are multiple targets, one for each file. Then all should be fine and you just need find_package(AdaptiveCpp).

Will let you know once I've tested on Polaris.

@TApplencourt
Copy link
Collaborator Author

Looks good mostly, a few nits. In general the code could do with a clang-format using the config that was added in #368

368 removed the clang-format? So you want to use it before it was removed?

Right now we just have clang format in

./External/computecpp-sdk/.clang-format
./External/Catch2/.clang-format

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.

3 participants