Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Change pattern to reflect the change in sycl-ls #256

Merged
merged 21 commits into from
Sep 24, 2021

Conversation

bso-intel
Copy link

In intel/llvm#3611, I changed the device type to lower case which was necessary to show the SYCL_DEVICE_FILTER prefix for each device listing from sycl-ls.
It caused several tests failures that expected 'GPU' instead of 'gpu'.

Signed-off-by: Byoungro So [email protected]

In intel/llvm#3611, I changed the device type to lower case which was necessary to show the SYCL_DEVICE_FILTER prefix for each device listing from sycl-ls.
It caused several tests failures that expected 'GPU' instead of 'gpu'.

Signed-off-by: Byoungro So <[email protected]>
@bso-intel bso-intel marked this pull request as ready for review May 17, 2021 18:59
@bso-intel bso-intel requested a review from smaslov-intel as a code owner May 17, 2021 18:59
Copy link

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

Why can't we make this, and other fields of SYCL_DEVICE_FILTER not case-sensitive?

@bso-intel
Copy link
Author

bso-intel commented May 17, 2021

Why can't we make this, and other fields of SYCL_DEVICE_FILTER not case-sensitive?

Currently, the implementation of std::ostream device_filter::operator<<() prints only lower-cases.
Users can set SYCL_DEVICE_FILTER whatever cases they want, but our test only look for specific patterns from the sycl-ls print output.
So, to make these tests pass, I had to change the pattern to lower cases.

@smaslov-intel
Copy link

Why can't we make this, and other fields of SYCL_DEVICE_FILTER not case-sensitive?

Currently, the implementation of std::ostream device_filter::operator<<() prints only lower-cases.
Users can set SYCL_DEVICE_FILTER whatever cases they want, but our test only look for specific patterns from the sycl-ls print output.
So, to make these tests pass, I had to change the pattern to lower cases.

I mean I'd like to see a RT change to accept upper-case values instead of this change.

@bso-intel
Copy link
Author

bso-intel commented May 17, 2021

Why can't we make this, and other fields of SYCL_DEVICE_FILTER not case-sensitive?

Currently, the implementation of std::ostream device_filter::operator<<() prints only lower-cases.
Users can set SYCL_DEVICE_FILTER whatever cases they want, but our test only look for specific patterns from the sycl-ls print output.
So, to make these tests pass, I had to change the pattern to lower cases.

I mean I'd like to see a RT change to accept upper-case values instead of this change.

Yes, RT accepts upper-case values (or any combination works).
But, when we print device filter, we only print the device type in lower case, which was used in sycl-ls.

Please note that this test is only trying to match the string pattern that 'sycl-ls' prints.
It is not testing what string SYCL_DEVICE_FILTER is accepting.

smaslov-intel
smaslov-intel previously approved these changes May 18, 2021
vladimirlaz
vladimirlaz previously approved these changes May 18, 2021
@bso-intel bso-intel dismissed stale reviews from vladimirlaz and smaslov-intel via 1f07119 June 22, 2021 00:35
@bso-intel bso-intel requested a review from a team as a code owner June 22, 2021 00:35
// CHECK-GPU: Device: gpu
// CHECK-CPU: Device: cpu

Choose a reason for hiding this comment

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

Why this line was moved? It does not look like intended change,

Copy link
Author

Choose a reason for hiding this comment

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

I did refactoring of get_devices(), and it returns CPU devices after GPU devices.

Signed-off-by: Byoungro So <[email protected]>
vladimirlaz
vladimirlaz previously approved these changes Jul 2, 2021
smaslov-intel
smaslov-intel previously approved these changes Aug 8, 2021
smaslov-intel
smaslov-intel previously approved these changes Sep 23, 2021
vladimirlaz
vladimirlaz previously approved these changes Sep 24, 2021
@bso-intel
Copy link
Author

@vladimirlaz ,
Could you merge this PR as soon as possible since intel/llvm#4247 is already merged?

@bso-intel bso-intel dismissed stale reviews from vladimirlaz and smaslov-intel via d385794 September 24, 2021 17:31
Signed-off-by: Byoungro So <[email protected]>
Signed-off-by: Byoungro So <[email protected]>
@vladimirlaz vladimirlaz merged commit 0ad1054 into intel:intel Sep 24, 2021
@bso-intel bso-intel deleted the filter branch September 28, 2021 21:33
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
…st-suite#256)

In intel#3611, I changed the device type to lower case which was necessary to show the SYCL_DEVICE_FILTER prefix for each device listing from sycl-ls.
It caused several tests failures that expected 'GPU' instead of 'gpu'.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants