Skip to content

[SYCL] Cleanup/refactor device_code_tests #14063

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

Closed
wants to merge 108 commits into from

Conversation

ianayl
Copy link
Contributor

@ianayl ianayl commented Jun 5, 2024

This PR:

  • Changes more lit tests in sycl/test/check_device_code to use SYCL_EXTERNAL functions in order to remove redundant host-side boilerplate code, and
  • Moves tests not verifying device code outside of the check_device_code folder.
  • Moves tests verifying device code that are outside of the check_device_code folder into the check_device_code folder

@ianayl ianayl requested a review from a team as a code owner June 5, 2024 23:02
@ianayl ianayl requested a review from aelovikov-intel June 5, 2024 23:02
@ianayl ianayl temporarily deployed to WindowsCILock June 5, 2024 23:29 — with GitHub Actions Inactive
@ianayl ianayl temporarily deployed to WindowsCILock June 6, 2024 00:20 — with GitHub Actions Inactive
}

// CHECK-ASPECTS: define dso_local spir_func void @{{.*}}kernel_name_2{{.*}} !sycl_declared_aspects ![[ASPECTS4:[0-9]+]]
// CHECK-SRCLOC: define dso_local spir_func void @{{.*}}kernel_name_2{{.*}} !srcloc ![[SRCLOC8:[0-9]+]] {{.*}}
SYCL_EXTERNAL [[sycl::device_has(sycl::aspect::gpu)]] void kernel_name_2() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might need both new and old code, actually. Someone from @intel/dpcpp-tools-reviewers should review changes in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

The attribute can be applied to both kernels and SYCL_EXTERNAL functions, I agree with @aelovikov-intel that we should have both variants tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split the test cases into 2 files, one for kernels and one for functions. Am I good to proceed with this?

@aelovikov-intel aelovikov-intel requested a review from a team June 6, 2024 21:49
@ianayl ianayl changed the title [SYCL] Change more check_device_code lit tests to use SYCL_EXTERNAL functions [SYCL] Cleanup/refactor device_code_tests Jun 6, 2024
aelovikov-intel pushed a commit that referenced this pull request Jun 6, 2024
…14088)

CODEOWNERS seems to be missing a line attributing
`sycl/test/check_device_code/matrix` tests to
intel/sycl-matrix-reviewers (As per [this
discussion](#14063 (comment))).
This PR remedies this.

Although, I noticed the current CODEOWNERS section for the matrix
reviewers uses paths; Let me know if I should use `sycl/**/matrix`
instead.
@ianayl
Copy link
Contributor Author

ianayl commented Jun 10, 2024

@AlexeySachkov Please excuse the ping, I've made some changes following your comments: Are these changes good, or do they need further revision?

Thanks in advance!

@ianayl
Copy link
Contributor Author

ianayl commented Jun 11, 2024

@aelovikov-intel Are these changes good? I believe I still need your approval before I can move onto CI/github workflow. Thanks!

@aelovikov-intel
Copy link
Contributor

@aelovikov-intel Are these changes good? I believe I still need your approval before I can move onto CI/github workflow. Thanks!

These are different approvals. I'll continue my PR review later today.

@ianayl ianayl temporarily deployed to WindowsCILock June 11, 2024 15:26 — with GitHub Actions Inactive
@ianayl ianayl temporarily deployed to WindowsCILock June 11, 2024 17:02 — with GitHub Actions Inactive
…ctor check_device_code/accessor_index.cpp to use SYCL_EXTERNAL functions
@ianayl ianayl requested review from a team as code owners June 11, 2024 20:53
@ianayl ianayl requested a review from steffenlarsen June 11, 2024 20:53
kbenzie and others added 12 commits June 13, 2024 15:13
Add section to the contribution guide detailing the current process for
integrating Unified Runtime updates into DPC++.
…ntel#14123)

Current implementation of profiling info for NOP barriers is
inconsistent
with other events from the same queue (e.g., if the previous event
started
after the barrier was submitted). To make them consistent while keeping
the optimization, we would need to duplicate the event on our side and
make the duplicate check and potentially use profiling info of its
previous event.

Instead, as the first step, disable the NOP optimization during
profiling
since profiling is known to incur a performance hit anyway. The proper
duplicate event approach can be implemented as a follow up if this
causes issues for users.

Partially reverts intel#12949
atomic_ref<T *> uses 64-bit atomics and it should be decorated with the
corresponding aspect.

fixes: intel#12743
I'm observing cache overflow when running heavy tests on OCL backend
with gpu. Clear cache in case of PI_ERROR_OUT_OF_HOST_MEMORY as well as
for PI_ERROR_OUT_OF_RESOURCES.
Using as reference: intel#11987
Scheduled igc dev drivers uplift

Co-authored-by: GitHub Actions <[email protected]>
The function is using the `operator=` before it's defined which can
cause some build failures:

```
build/include/sycl/ext/oneapi/bfloat16.hpp:98:19: error: no match for ‘operator=’ (operand types are ‘sycl::_V1::ext::oneapi::bfloat16’ and ‘float’)
   98 |     dst[i] = src[i];
      |                   ^
```

Moving it after the bfloat16 class definition fixes it.
Bumps [braces](https://github.com/micromatch/braces) from 3.0.2 to
3.0.3.
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/micromatch/braces/commit/74b2db2938fad48a2ea54a9c8bf27a37a62c350d"><code>74b2db2</code></a>
3.0.3</li>
<li><a
href="https://github.com/micromatch/braces/commit/88f1429a0f47e1dd3813de35211fc97ffda27f9e"><code>88f1429</code></a>
update eslint. lint, fix unit tests.</li>
<li><a
href="https://github.com/micromatch/braces/commit/415d660c3002d1ab7e63dbf490c9851da80596ff"><code>415d660</code></a>
Snyk js braces 6838727 (<a
href="https://redirect.github.com/micromatch/braces/issues/40">#40</a>)</li>
<li><a
href="https://github.com/micromatch/braces/commit/190510f79db1adf21d92798b0bb6fccc1f72c9d6"><code>190510f</code></a>
fix tests, skip 1 test in test/braces.expand</li>
<li><a
href="https://github.com/micromatch/braces/commit/716eb9f12d820b145a831ad678618731927e8856"><code>716eb9f</code></a>
readme bump</li>
<li><a
href="https://github.com/micromatch/braces/commit/a5851e57f45c3431a94d83fc565754bc10f5bbc3"><code>a5851e5</code></a>
Merge pull request <a
href="https://redirect.github.com/micromatch/braces/issues/37">#37</a>
from coderaiser/fix/vulnerability</li>
<li><a
href="https://github.com/micromatch/braces/commit/2092bd1fb108d2c59bd62e243b70ad98db961538"><code>2092bd1</code></a>
feature: braces: add maxSymbols (<a
href="https://github.com/micromatch/braces/issues/">https://github.com/micromatch/braces/issues/</a>...</li>
<li><a
href="https://github.com/micromatch/braces/commit/9f5b4cf47329351bcb64287223ffb6ecc9a5e6d3"><code>9f5b4cf</code></a>
fix: vulnerability (<a
href="https://security.snyk.io/vuln/SNYK-JS-BRACES-6838727">https://security.snyk.io/vuln/SNYK-JS-BRACES-6838727</a>)</li>
<li><a
href="https://github.com/micromatch/braces/commit/98414f9f1fabe021736e26836d8306d5de747e0d"><code>98414f9</code></a>
remove funding file</li>
<li><a
href="https://github.com/micromatch/braces/commit/665ab5d561c017a38ba7aafd92cc6655b91d8c14"><code>665ab5d</code></a>
update keepEscaping doc (<a
href="https://redirect.github.com/micromatch/braces/issues/27">#27</a>)</li>
<li>Additional commits viewable in <a
href="https://github.com/micromatch/braces/compare/3.0.2...3.0.3">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=braces&package-manager=npm_and_yarn&previous-version=3.0.2&new-version=3.0.3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the
[Security Alerts page](https://github.com/intel/llvm/network/alerts).

</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…tly (intel#12872)

This PR refactors the builtin fence helper macro for AMDGPU to take in
and process the order semantic explicitly because that is the only
semantic argument accepted by the amdgcn builtin.

Additionally, makes the `None` (Monotonic) order semantic which maps to
C++/SYCL's `relaxed` to be a no-op instead of falling back to the
previous `acq_rel` default order.

---------

Co-authored-by: Kenneth Benzie (Benie) <[email protected]>
The rotate functions are technically c++20 and MSVC hasn't implemented
them yet.

Signed-off-by: Sarnie, Nick <[email protected]>
…ntel#14151)

One of the models that is used for specifying the device architecture
for spir64_gen is to use the -Xsycl-target-backend "-device arg" syntax
on the command line.

Hook up the ability to scan the target backend values to embed the
proper information in the packaged binary when using the new offload
model.
@ianayl ianayl force-pushed the SYCLEXTERNALdevicechecks branch from 88cba5e to abfd64a Compare June 13, 2024 22:15
@ianayl ianayl requested review from a team, tfzhu and bader as code owners June 13, 2024 22:15
@ianayl
Copy link
Contributor Author

ianayl commented Jun 13, 2024

Hey all,

Terribly sorry for the mess here: I was trying to remove certain commits from this branch and didn't know rebasing would cause everyone in a prior merge to be notified. I will be opening up a new branch for this instead.

@ianayl ianayl closed this Jun 13, 2024
@bader
Copy link
Contributor

bader commented Jun 13, 2024

@ianayl, please, reset your branch to 88cba5e and re-open the pull request.
Avoid using rebase and just put fixes as separate commits on top of your branch.

@ianayl
Copy link
Contributor Author

ianayl commented Jun 13, 2024

@bader My bad, that is what I am currently doing. Thanks for your patience!

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.