Skip to content

[SYCL] Adding SYCL backend to support for Intel XPUs - #215

Open
abagusetty wants to merge 8 commits into
PASSIONLab:mainfrom
abagusetty:xpu-support
Open

abagusetty wants to merge 8 commits into
PASSIONLab:mainfrom
abagusetty:xpu-support

Conversation

@abagusetty

@abagusetty abagusetty commented Sep 3, 2026

Copy link
Copy Markdown

Major changes:

  • Added feature support to target Intel GPUs (on ANL Aurora) via SYCL backend and generalize device selection so CUDA, HIP and SYCL are handled seamlessly

@vbharadwaj-bk

Copy link
Copy Markdown
Member

Great feature, thanks! This will take some time to review and test - so bear with us, but with some back and forth we'll see where we land.

Kernels are generated as SYCL free functions and compiled at runtime through
the oneAPI kernel compiler. The Jinja templates are shared with CUDA and HIP
via templates/sycl_compat.cuh; the backend is now a string rather than an
is_hip boolean, and JITKernel::execute also takes argument sizes.
Mirrors the CUDA job. GitHub offers no Intel GPU runner, so only the build is
exercised.
The stable-ABI headers the extension includes are not all present before 2.10,
which the precompiled-extension check in extlib has required all along. The
SYCL guard now enforces the same floor.
Upstream PASSIONLab#217 replaced HIP's __threadfence_block() with the amdgcn wave
barrier, so no template emits the token any more.
@vbharadwaj-bk

Copy link
Copy Markdown
Member

Got a review in progress for this, about 20/50 files in; thanks for the patience, it's going

@abagusetty

Copy link
Copy Markdown
Author

Got a review in progress for this, about 20/50 files in; thanks for the patience, it's going

Thanks @vbharadwaj-bk Appreciate for going through this. Not a small PR!

@vbharadwaj-bk vbharadwaj-bk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@abagusetty I read every one of the + 1375 -270 line diff 😅 actually, I think this will work nicely with our current setup and I have left comments in the files. Much of this is mechanical; high-level, so you can have an LLM take care of some of this

  1. LLMs tend to leave a lot of inline / block comments. Let's remove 99% of them, I think there were only one or two comments throughout the entire diff that I felt were necessary to understand something.

  2. Let's roll back the changes to group_mm / symmetric contraction, as there is another PR in the queue to address the BLAS dependency and this should no longer be required. Or worst case, we won't support symmetric contraction on SYCL just yet.

  3. There are just a few requests for code logic changes in certain files.

  4. Let's hold off on the README / changelog things for now, we'll work with you on those once this batch of changes is in.

I didn't see any big logical gaps, but I also haven't tested this for regression on the existing backends; I will do that once this round of edits is done. In case there are bugs filed specifically for the SYCL backend, we will probably refer them to you to address, since we don't have Intel hardware. Are you ok with this, at least for a little while?

^^ Once this round of edits is done, and if we can refer bugs to you for some period of time, then I think with one more review, we can merge this.

@@ -0,0 +1,9 @@
# The SYCL backend follows the project's existing PyTorch >= 2.10 floor.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's remove this comment; LLMs tend to add a lot of comments, so I'll abbreviate requests to remove comments and keep code self-documenting with --selfdocument or --nodocument

cache: 'pip'
cache-dependency-path: '**/requirements_sycl_ci.txt'

# The SYCL backend compiles the extension with the oneAPI DPC++ driver

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--selfdocument

# No Intel GPU is attached to this runner; only the build is exercised.
- name: Install oneAPI DPC++ and oneMKL
run: |
wget -qO- https://apt.repos.intel.com/intel-gpg-keys/GPG-PUB-KEY-INTEL-SW-PRODUCTS.PUB \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm - have you timed how long this takes, perhaps on your fork? If it takes too long, not thrilled about including it in the CI since our minutes are limited.

Comment thread docs/index.rst
`OpenEquivariance <https://github.com/PASSIONLab/OpenEquivariance>`_ is a CUDA and
HIP kernel generator for the Clebsch-Gordon
`OpenEquivariance <https://github.com/PASSIONLab/OpenEquivariance>`_ is a CUDA,
HIP, and SYCL kernel generator for the Clebsch-Gordon

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's hold off README changes until the PR is in place; want to make sure that people know SYCL is in a beta state, and will receive less support (probably) from us as maintainers.

Determines which GPU backend this PyTorch build targets.

Returns one of ``"cuda"``, ``"hip"`` or ``"sycl"``. HIP builds report a
``torch.version.cuda`` of ``None``, so HIP must be tested first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--nodocument

Builds the Jinja environment used to render the kernel templates.

:param backend: one of ``"cuda"``, ``"hip"`` or ``"sycl"``.
:param warp_size: size of a warp / wavefront / sub-group. Only consulted by

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm - warp size is needed by HIP as well.. but maybe that's a separate bug we need to address.

__launch_bounds__({{schedule.launch_config.num_threads}})
{%- endmacro %}

{# Declares the per-block shared memory buffer `s`. CUDA and HIP size the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--nodocument

)
add_stable_extension(oeq_stable_sycl SYCL_BACKEND "${SYCL_LINK_LIBS}")

# -fsycl is required on both the compile and the link line.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--nodocument

@@ -0,0 +1,115 @@
{#
Compatibility shim that lets the CUDA/HIP-flavored kernel templates compile as

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--nodocument

operator size_t() const { return x; }
};

static inline OeqIndex1D oeq_thread_idx() { return {twi::get_nd_item<1>().get_local_id(0)}; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm - I feel like a lot of these functions should perhaps be prefixed _sycl_thread_idx, or similar? Prefixing with oeq seems to imply distinct backends, whereas these functions are only called in the SYCL path.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto for all remaining functions in this file.

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.

2 participants