Skip to content

Conversation

@AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Oct 25, 2024

The main idea behind this (and other PRs I will eventually submit) is that we shouldn't use SYCL_UR_TRACE in our unit-tests unless absolutely necessary (like testing shutdown sequence).

The reason behind this is the fact that we have unit-tests infrastructure which is more than capable of ensuring that we called the right UR APIs with the right arguments. There is no need to waste our CI time on compiling and running the same test on different devices if we can easily test the same via our UR mock infrastructure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for ESIMD reviewers: even though the test also does some computations in kernels, I assume that they are not unique to this test and was merely done to avoid submitting an empty kernel or just by copying an existing test as a baseline.

Therefore, I'm completely removing it in favor of unit-test-level checks that we pass the right JIT compilation flag down to UR program build APIs.

Please speak up if you disagree and the test should still be preserved for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah the computation is definitely not important, the point of the test is to check the flag to IGC.

// REQUIRES: gpu && linux && (opencl || level_zero)

// RUN: %{build} -o %t.out
// RUN: env NEOReadDebugKeys=1 CreateMultipleRootDevices=3 SYCL_UR_TRACE=2 %{run} %t.out
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lit captures stdout/stderr, so why waste our machine resources for something that is not used anyways?

@AlexeySachkov AlexeySachkov marked this pull request as ready for review October 30, 2024 10:15
@AlexeySachkov AlexeySachkov requested review from a team as code owners October 30, 2024 10:15
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.

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.

this is cool, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah the computation is definitely not important, the point of the test is to check the flag to IGC.

sycl::range{1024}, [=](sycl::id<1>) /* SYCL_ESIMD_KERNEL */ {});
});

EXPECT_EQ(BuildOpts,
Copy link
Contributor

Choose a reason for hiding this comment

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

wow cool!

@AlexeySachkov AlexeySachkov merged commit dd7446c into intel:sycl Oct 30, 2024
14 checks passed
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.

3 participants