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

[SYCL][Graph][Doc] Add SYCL-Graph usage guide and example doc #379

Closed
wants to merge 35 commits into from

Conversation

Bensuo
Copy link
Collaborator

@Bensuo Bensuo commented Jul 29, 2024

  • Move examples from spec to reduce bloat
  • Adds some usage guidelines for common scenarios
  • Remove reductions from examples since they are not supported

sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
@Bensuo Bensuo force-pushed the ben/graph-usage-guide branch 2 times, most recently from 80f6e78 to bc92151 Compare July 30, 2024 14:56
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM as a starting point for this document we can improve on over time.

sycl/doc/SYCLGraphUsageGuide.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@julianmi julianmi left a comment

Choose a reason for hiding this comment

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

LGTM

sarnex and others added 15 commits August 1, 2024 14:51
The problem was this was a runtime `if` and not `if constexpr`. This
function was copy-pasted from the ESIMD headers, and the headers one was
already fixed to be `if constexpr`, so just use that.

Closes: intel#14877

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
)

Layout is slightly different on systems like RHEL8 with older libstdc++.
Adjust regexp accordingly.
…14883)

Returning a std::string from device.cpp to device.hpp will cross the ABI
boundary.
So, we need to neutralize it via detail::string.
In the same vein as intel#14273, this PR prevents exceptions from leaking in
additional destructors caught by Coverity.

I'd like to draw attention to `device_impl.cpp` however: A comment was
left suggesting that exceptions in the `device_impl` destructor be added
to the asynchronous exceptions list. Given that devices are usually
destroyed during shutdown, adding exceptions to the exceptions list
doesn't seem to make sense, as there would be nothing to handle the
exceptions anyway.

However, if this understanding is incorrect, and I should still add
exceptions to an asynchronous exceptions list, please let me know.
Thanks!
…TOR (intel#14865)

replaced XFAIL approach with "not" usage in RUN lines

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
This patch introduces `map_external_linear_memory` to enable mapping
interop memory to linear USM, returning a `void *`.

The PI function `piextMemMapExternalLinearMemory` has been added to
enable this in the PI layer.

A Vulkan test case has been added to test this functionality.

---------

Co-authored-by: Duncan Brawley <duncan.brawley@codeplay.com>
Some files mistakenly had the wrong permissions. This removes all
execute permissions from asciidoc, .cpp or .hpp files.
Add option "-fsycl-allow-device-dependencies" to enable support for
dynamic linking.

Also:
1. No functions are importable without
"-fsycl-allow-device-dependencies"
2. Deal with SYCL_EXTERNAL header decls that have lost SYCL_EXTERNAL
attribute in LLVM IR
3. SPIRV/SYCL/ESIMD builtins cannot be an imported function

Tested in three E2E test cases.

Minor change:
Change SYCL-EXTERNAL to SYCL_EXTERNAL in testcase comment.

---------

Signed-off-by: Lu, John <john.lu@intel.com>
Co-authored-by: Marcos Maronas <marcos.maronas@intel.com>
…intel#14872)

- fix for intel#14444

Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>
Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>
Co-authored-by: Peter Žužek <peterzuzek@gmail.com>
`map.insert` doesn't insert values if the set already contains them.
This can happen when UR/PI happens to reuse the same program pointer
that it used for a previous program.

--

This was causing some tests in the PI 2 UR conversion to randomly fail,
including at least intel#14765 .

Fixes intel#14819.
jsji and others added 19 commits August 2, 2024 20:16
This to align with what we had before PI removal.
We need at least the /EHsc flag to be able to build UR in some Windows
systems.
Fix debug build issue
 [-Werror,-Wmicrosoft-include] llvm\include\sycl\handler.hpp:52:
intel#14819 has been fixed, so turning
the tests back on.
See intel#14932

These tests were added 4 years ago with XFAIL and have never been
enabled.
…#14875)

Implementation design explaining those changes in a bigger picture can
be found in intel#10540

Key things implemented here:
- device code split to outline virtual functions into separate device
images
- emission of new properties for virtual functions
- generation of `calls-indirectly` LLVM IR attribute for kernels that
construct objects with virtual functions, but don't do calls
- device image manipulations to cleanup or preserve virtual functions
depending on a device image

Even though those pieces are technically independent from each other, it
is hard to split them apart into separate PRs, because they all have to
be either present or absent for existing E2E tests for virtual functions
to work.
…ion (intel#14827)

This option allows to save generated SPIRV files in the specified
directory.
Consists of two parts:
1. Fix the setting of `-Werror` on Windows in general for E2E tests
* Just appending to `SYCL_E2E_CLANG_CXX_FLAGS` adds an extra semicolon
that causes the command line to fail
3. Address remaining `-Werror` failures in Bindless Images E2E tests
    * Add return from function, even though there's an `assert(false)`
Those tests currently fail on AMD devices. To enable them,
[sycl_ext_oneapi_free_function_kernels.asciidoc](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_free_function_kernels.asciidoc)
needs to be investigated how to make it work for HIP backend.
This patch refactors the way we lower SYCL attributes and properties to
NVVM annotations, through function attributes and metadata. It unifies
the flow better with the SPIR-V paths at the same time.

Previously we had:

1. Clang handling function attributes in two places:
1. CodeGenFunction did generic lowering of attributes to function
metadata
2. NVPTXTargetCodeGenInfo did its own additional lowering of attributes
to NVVM annotations
2. Kernel properties being handled in clang. NVPTXTargetCodeGenInfo
lowered kernel properties, which had already been converted to function
attributes, to NVVM annotations.
3. Kernel properties for HIP/CUDA *not* being turned into function
metadata. Because function metadata is how the ComputeModuleRuntimeInfo
library creates runtime info, this meant that target backends couldn't
act on properties, as they were lost during lowering.

Fundamentally, clang is not the correct place for handling SYCL kernel
properties as it unnecessarily touches the front-end for what is really
library code.

With this patch, we now have:

1. Clang handles function attributes in one place, lowering to function
metadata
2. SYCLLowerIR lowers kernel properties to the same function metadata:
now working for HIP and CUDA kernels too. Because function metadata is
consistently present for kernel properties, they behave as expected on
HIP/CUDA targets now.
3. A new pass converts function metadata to NVVM annotations, replacing
the code we've taken out of clang.
Some system headers included are not needed anymore.
For reference_argument.cpp, the test `RUN` line was wrong, and I added
the real error message to the `CHECK` string.

For `simd32_platform_error.cpp`, all Intel GPUs we test on report as
support SIMD32 (for the ones that don't in hardware, it is probably
emulated but it definitely works), so they don't throw this error so we
can't test it, so just remove the test.

Closes: intel#14643
Closes: intel#14644

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
The test was disabled on Linux but the error message shows it failed on
Windows:

```
# | ...\llvm\install\bin\clang-offload-wrapper: error: '...\AppData\Local\Temp\lit-tmp-36f_juo7\ctor_load_usm_fp_extra-167ca4_esimd_14.sym': Operation did not complete successfully because the file contains a virus or potentially unwanted software.
```

`AppData` is a Windows folder and the linked PRs show it failing in the
Windows job (example
[here](https://github.com/intel/llvm/actions/runs/9994419232/job/27630212012))

The test isn't failing on Windows where the issue was reported to occur,
so just remove the wrong `UNSUPPORTED` line.

Closes: intel#14650

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
We were using the `x` prefix to the `RUN` lit command to comment out a
`RUN` check. However, LIT still detects this as a `RUN` command, which
illustrated in a CI fail of this test recently
https://github.com/intel/llvm/actions/runs/10198164930/job/28215254220

Fixed by changing `xRUN` to `RUNx` which I've verified locally is not
detected as a `RUN` command by LIT, and can be used to comment the `RUN`
command out.

Relates to intel#14473
This patch renames the directory (`sycl-fusion` to `sycl-jit`) as well
as the involved library names (following the same principle: `fusion` to
`jit`).
To keep the user facing names consistent, buildbot switches were updated.
And finally the codeowners file had to reflect the directory change.
4f86ab replaced `insert` with `[]` in order to fix a pointer re-use
issue, however that was not the correct fix. Instead, a multimap was
incorrectly converted to a regular map during the PI->UR conversion.

This change reverts the rest of 4f86ab (besides the test, which is still
valid), and converts NativePrograms back to a multimap.

---

Thanks to @AlexeySachkov for finding the real issue in
intel#14873 (comment)
e.g.
```
$ ninja check-sycl-dumps
 [...]
 # .---command stderr------------
 # | 'FileCheck': command not found
 # `-----------------------------
 # error: command failed with exit status: 127
```
- Move examples from spec to reduce bloat
- Adds some usage guidelines for common scenarios
- Remove reductions from examples since they are not supported
@Bensuo
Copy link
Collaborator Author

Bensuo commented Aug 6, 2024

Upstream PR: intel#14965

@Bensuo Bensuo closed this Aug 6, 2024
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.