Skip to content

[SYCL][TEST] Remove check_has.cpp dependency on metadata order #11809

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

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Nov 8, 2023

Previously, we committed #9642 aiming to break dependency on metadata order for check_has.cpp but the commit was broken unexpectedly. This PR re-lands it.

@jinge90 jinge90 requested a review from a team as a code owner November 8, 2023 06:07
@jinge90
Copy link
Contributor Author

jinge90 commented Nov 8, 2023

Hi, @steffenlarsen and @maarquitos14
Previously, we added declarations for "hypotf" when oneAPI compiler is used on Windows platform but the change lead to device_has.cpp lit test failure due to meta data order is changed (aspects and srcloc), so we need to fix the test.
Previous test fix is: #9642

Signed-off-by: jinge90 <ge.jin@intel.com>
Signed-off-by: jinge90 <ge.jin@intel.com>
@maarquitos14
Copy link
Contributor

Hi, @steffenlarsen and @maarquitos14 Previously, we added declarations for "hypotf" when oneAPI compiler is used on Windows platform but the change lead to device_has.cpp lit test failure due to meta data order is changed (aspects and srcloc), so we need to fix the test. Previous test fix is: #9642

@jinge90 why was it reverted earlier? Did it cause any issue?

@jinge90
Copy link
Contributor Author

jinge90 commented Nov 8, 2023

Hi, @steffenlarsen and @maarquitos14 Previously, we added declarations for "hypotf" when oneAPI compiler is used on Windows platform but the change lead to device_has.cpp lit test failure due to meta data order is changed (aspects and srcloc), so we need to fix the test. Previous test fix is: #9642

@jinge90 why was it reverted earlier? Did it cause any issue?

Hi, @maarquitos14
It was not reverted but #10317 changed the test and removed the 2-time checks, our previous test didn't cause any issue. Maybe the author thought that 2-time checks seemed to be unnecessary.
Thanks very much.

@maarquitos14
Copy link
Contributor

Hi, @steffenlarsen and @maarquitos14 Previously, we added declarations for "hypotf" when oneAPI compiler is used on Windows platform but the change lead to device_has.cpp lit test failure due to meta data order is changed (aspects and srcloc), so we need to fix the test. Previous test fix is: #9642

@jinge90 why was it reverted earlier? Did it cause any issue?

Hi, @maarquitos14 It was not reverted but #10317 changed the test and removed the 2-time checks, our previous test didn't cause any issue. Maybe the author thought that 2-time checks seemed to be unnecessary. Thanks very much.

I see, thanks for explaining.

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.

@jinge90 jinge90 requested a review from a team November 9, 2023 02:03
@jinge90
Copy link
Contributor Author

jinge90 commented Nov 9, 2023

Hi, @intel/llvm-gatekeepers
Could you help merge this patch?
Thanks very much.

@steffenlarsen steffenlarsen merged commit 1f4ff10 into intel:sycl Nov 9, 2023
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