Skip to content

Conversation

maksimsab
Copy link
Contributor

This change extracts handleESIMD from sycl-post-link to SYCLPostLink component for reuse in NewOffloading compilation flow in clang and for reuse in sycl-jit.

The body of handleESIMD function is refactored for better readability.

The documentation of lowerESIMDConstructs is fixed and the argument name is changed to more straightforward.

This change extracts handleESIMD from sycl-post-link to SYCLPostLink
component for reuse in NewOffloading compilation flow in clang and for
reuse in sycl-jit.

The documentation of lowerESIMDConstructs are fixed and the argument
name is changed to more straightforward.
@maksimsab maksimsab requested a review from a team as a code owner May 27, 2025 16:04
@maksimsab maksimsab requested a review from sarnex May 27, 2025 16:04
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM overall. Minor nit.
I also have a question. Will it be reasonable to add a unittest for testing this functionality?
(location: llvm/unittests)
Not a blocker though.

Thanks

@maksimsab
Copy link
Contributor Author

Speaking of the testing, different parts are being tested by LIT tests by sycl-post-link. Linking is tested independently from sycl. Lowering is tested by LITs. The control flow of handleESIMD is difficult to test by unittests.

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

LGTM! Just a few nits.

@asudarsa
Copy link
Contributor

Hi @maksimsab
I had a look at the build fails. Not really clear what the issue is. Did you figure it out?
Thanks

@maksimsab
Copy link
Contributor Author

@asudarsa CI issues are really not related to this PR, I can see similar issues in #19022 .

@intel/llvm-gatekeepers Can we merge this?

@dm-vodopyanov
Copy link
Contributor

@asudarsa CI issues are really not related to this PR, I can see similar issues in #19022 .

@intel/llvm-gatekeepers Can we merge this?

Could you please create issues against these failures?

Copy link
Contributor

github-actions bot commented Sep 8, 2025

@intel/llvm-gatekeepers please consider merging

@sarnex
Copy link
Contributor

sarnex commented Sep 8, 2025

@maksimsab is this actually ready for merge?

Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

@sarnex
Copy link
Contributor

sarnex commented Sep 11, 2025

@maksimsab x2 is this actually ready for merge?

@maksimsab
Copy link
Contributor Author

@sarnex thanks for pointing out!

@intel/llvm-gatekeepers Can we merge this please?

@uditagarwal97 uditagarwal97 merged commit cb8b521 into intel:sycl Sep 12, 2025
27 checks passed
Maetveis added a commit to Maetveis/llvm that referenced this pull request Sep 22, 2025
Starting from intel#18684 in debug/release
with asserts builds entry points are always dumped in ESIMDPostSplitProcessing.
This is meant to be a debugging tool, but now it is spamming build
logs.
intel#18684 is marked as NFC, so my
assumption is that this was unintentional.
Guard the dump with a variable (disabled by default) so that it can be
enabled for debugging, the same way as it was in sycl-post-link.cpp prior
to the code move.

FYI: `LLVM_ENABLE_DUMP` is an option documented to
> Enable dump functions even when assertions are disabled

(from [llvm/CMakeLists.txt:698](https://github.com/llvm/llvm-project/blob/32b1f167fbee28debc7527b939a6764575c854a4/llvm/CMakeLists.txt#L698))

and its typical usage is to guard the definitions of dump functions, e.g.:
```cpp
  LLVM_DUMP_METHOD void dump() const;
```
and not to guard **calls** to such functions.
Maetveis added a commit to Maetveis/llvm that referenced this pull request Sep 22, 2025
Starting from intel#18684 in debug/release
with asserts builds entry points are always dumped in ESIMDPostSplitProcessing.
This is meant to be a debugging tool, but now it is spamming build
logs.
intel#18684 is marked as NFC, so my
assumption is that this was unintentional.
Guard the dump with a variable (disabled by default) so that it can be
enabled for debugging, the same way as it was in sycl-post-link.cpp prior
to the code move.

FYI: `LLVM_ENABLE_DUMP` is an option documented to
> Enable dump functions even when assertions are disabled

(from [llvm/CMakeLists.txt:698](https://github.com/llvm/llvm-project/blob/32b1f167fbee28debc7527b939a6764575c854a4/llvm/CMakeLists.txt#L698))

and its typical usage is to guard the definitions of dump functions, e.g.:
```cpp
class Foo {
// ...
  void dump() const;
}
```
and not to guard **calls** to such functions.
sarnex pushed a commit that referenced this pull request Sep 24, 2025
…20150)

Starting from #18684 in debug/release
with asserts builds entry points are always dumped in
ESIMDPostSplitProcessing. This is meant to be a debugging tool, but now
it is spamming build logs.
#18684 is marked as NFC, so my
assumption is that this was unintentional.
Guard the dump with a variable (disabled by default) so that it can be
enabled for debugging, the same way as it was in sycl-post-link.cpp
prior to the code move.

FYI: `LLVM_ENABLE_DUMP` is an option documented to
> Enable dump functions even when assertions are disabled

(from
[llvm/CMakeLists.txt:698](https://github.com/llvm/llvm-project/blob/32b1f167fbee28debc7527b939a6764575c854a4/llvm/CMakeLists.txt#L698))

and its typical usage is to guard the definitions of dump functions,
e.g.:
```cpp
class Foo {
// ...
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
  void dump() const;
#endif
}
```
and not to guard **calls** to such functions.
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.

6 participants