Skip to content

Add negative tests for inline asm #2406

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

Conversation

fadeeval
Copy link
Contributor

@fadeeval fadeeval commented Sep 1, 2020

Add tests for verification of error messages for inline assembly feature.

Signed-off-by: Aleksander Fadeev [email protected]

@fadeeval fadeeval requested review from smaslov-intel and a team as code owners September 1, 2020 17:29
@fadeeval fadeeval force-pushed the private/fadeeval/asm_negative_tests branch from 09b3fb6 to 8f7b5d2 Compare September 1, 2020 17:32
Added new parameter to launchInlineASMTests to be able to get the
exceptions out of it to check their content in negative tests.

Updated spelling of `reqd_sub_group_size` attribute to avoid using
deprecated variant of it.
// RUN: %clangxx -fsycl %s -DINLINE_ASM -o %t.out
// RUN: not %t.out 2>&1 | FileCheck %s

// CHECK: Build succeeded
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why do we have tests that expected to be successfully built in negative_tests directory?

If these tests require particular HW to catch device program build error, than I suggest to remove them as they are too fragile, IMHO

smaslov-intel
smaslov-intel previously approved these changes Sep 2, 2020
Copy link
Contributor

@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.

LGTM, but I am not familiar with GPU asm. BTW, where it is specified which GPU HW to run on?

@@ -0,0 +1,43 @@
// UNSUPPORTED: cuda
// REQUIRES: gpu,linux
// RUN: %clangxx -fsycl %s -DINLINE_ASM -o %t.out
Copy link
Contributor

@smaslov-intel smaslov-intel Sep 2, 2020

Choose a reason for hiding this comment

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

Why do you need this INLINE_ASM macro since you seem to always define it?

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 remained from positive tests, I will delete it.

Signed-off-by: Aleksander Fadeev <[email protected]>
Signed-off-by: Aleksander Fadeev <[email protected]>
@fadeeval fadeeval force-pushed the private/fadeeval/asm_negative_tests branch from 9e81046 to 5afddf6 Compare September 3, 2020 07:41
smaslov-intel
smaslov-intel previously approved these changes Sep 3, 2020
Copy link
Contributor

@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.

LGTM

Signed-off-by: Aleksander Fadeev <[email protected]>
@fadeeval fadeeval force-pushed the private/fadeeval/asm_negative_tests branch from e2b6861 to f4fbcb0 Compare September 3, 2020 09:56
@fadeeval
Copy link
Contributor Author

fadeeval commented Sep 3, 2020

Don't merge please yet.

Signed-off-by: Aleksander Fadeev <[email protected]>
@fadeeval
Copy link
Contributor Author

fadeeval commented Sep 3, 2020

I won't fix clang formatting on purpose.

@fadeeval
Copy link
Contributor Author

fadeeval commented Sep 3, 2020

The PR can be merged now

@romanovvlad romanovvlad merged commit 17eb598 into intel:sycl Sep 3, 2020
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
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