Skip to content

Conversation

@aelovikov-intel
Copy link
Contributor

No description provided.

@aelovikov-intel aelovikov-intel requested review from a team as code owners October 4, 2023 19:35
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock October 4, 2023 19:37 — with GitHub Actions Inactive
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

Wow, this is cool, thanks! ESIMD test changes LGTM

@@ -1,3 +1,4 @@
// REQUIRES-INTEL-DRIVER: lin: 27202, win: 101.4677
Copy link
Contributor

@v-klochkov v-klochkov Oct 4, 2023

Choose a reason for hiding this comment

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

I agree that is really great feature. Thank you for adding it!

The only question from me: is this new filter completely disables the test or only '%{run}' portion of it?

Many tests are very useful as compile-only tests too. Otherwise, when the driver uplift comes, we may see not the expected runtime pass, but compilation fails caused by series of changes while the test was OFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It skips completely. The reason I've done it is because compilation might be affected by the driver too (e.g., AOT compilation crash). I think that is pretty common for the Matrix project/tests (which triggered this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, thinking more about it, the argument above might not be true. However, I'd really hate to report PASS for something that hasn't been executed.

Copy link
Contributor

@v-klochkov v-klochkov Oct 4, 2023

Choose a reason for hiding this comment

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

The old non-centralized checks in ESIMD monitored compile-time regressions, which was really good thing.

If only AOT compilation needs skip, then the best solution (IMO) would be to introduce %{build-aot} that is skipped if the driver doesn't meet the requirement. I have no idea though how difficult is to introduce %{build-aot}
So, if driver is too old:
%{build} - runs
%{build-aot} - skipped
%{run} - skipped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How often have you caught the problems on a test with a runtime skip that could not be observed on other tests? Was it in CI or locally? If locally, was the new/good driver available to you?

Copy link
Contributor

@v-klochkov v-klochkov Oct 4, 2023

Choose a reason for hiding this comment

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

Cannot say how often. Perhaps it is the small chance to have overlooked regression.
Compile-time regression may happen when someone else does the changes, e.g. pulls from llvm.org and simply relies on CI, while CI might not catch it.

Don't consider my comments as blocking. The new centralized filter is great in general, even though it has some cons if it skips %{build}.

Copy link
Contributor

@sarnex sarnex Oct 4, 2023

Choose a reason for hiding this comment

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

If we are worried about it the ESIMD team could find what tests are currently skipped in CI due to old driver and create compile-only tests for them if they have APIs that aren't locked down elsewhere.

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 don't like implicit skips because the give you a false sense of all tests passing. UNSUPPORTED is much better in this regard. I think the right way to enable compilation is to create a fake second test that will compile-only the one disabled with this new markup:

# a.compile.cpp
// RUN: %{build} %S/a.cpp -o %t.out
#a.cpp
// REQUIRES-INTEL-DRIVER: ...
// NOTE: Remove a.compile.cpp once the above line is removed.
...

If we think a.cpp is important enough

@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock October 4, 2023 19:52 — with GitHub Actions Inactive
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀

@aelovikov-intel
Copy link
Contributor Author

aelovikov-intel commented Oct 5, 2023

I don't think the following is caused by my changes, but I'll restart just in case.

# Win GEN12
Failed Tests (5):
  SYCL :: ESIMD/dword_local_accessor_atomic_smoke.cpp
  SYCL :: ESIMD/dword_local_accessor_atomic_smoke_cmpxchg.cpp
  SYCL :: ESIMD/dword_local_accessor_atomic_smoke_cmpxchg_scalar_off.cpp
  SYCL :: ESIMD/dword_local_accessor_atomic_smoke_scalar_off.cpp
  SYCL :: ESIMD/ext_math_saturate.cpp

Edit: Turns out they were - the included other .cpp files that I had modified. Needed to adjust those as well.

@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock October 5, 2023 16:09 — with GitHub Actions Inactive
They `#include` other files that have been transitioned. Need to do the
same for those 5 as well.
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock October 5, 2023 18:51 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock October 5, 2023 19:32 — with GitHub Actions Inactive
@aelovikov-intel
Copy link
Contributor Author

I don't think

Failed Tests (1):
  SYCL :: syclcompat/memory/memcpy_3d2.cpp

on Intel Linux GEN12 GPU is somehow related to this change.

@aelovikov-intel aelovikov-intel merged commit c8848b7 into intel:sycl Oct 5, 2023
@aelovikov-intel aelovikov-intel deleted the driver-version branch October 5, 2023 20:30
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Oct 10, 2023
`test-e2e/InvokeSimd/Regression/ImplicitSubgroup/slm_load_store.cpp`
wasn't updated together with
`test-e2e/InvokeSimd/Regression/slm_load_store.cpp` that it includes in
intel#11427. Do it now.
aelovikov-intel added a commit that referenced this pull request Oct 10, 2023
`test-e2e/InvokeSimd/Regression/ImplicitSubgroup/slm_load_store.cpp`
wasn't updated together with
`test-e2e/InvokeSimd/Regression/slm_load_store.cpp` that it includes in
#11427. Do it now.
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.

5 participants