Skip to content

Conversation

vinser52
Copy link
Contributor

No description provided.

@vinser52
Copy link
Contributor Author

@intel/llvm-reviewers-runtime, PR is ready for review.

Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

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

LGTM

@vinser52
Copy link
Contributor Author

@intel/llvm-gatekeepers PR is ready for merge

@@ -0,0 +1,26 @@
// RUN: %clangxx -fsycl -c -fno-color-diagnostics -Xclang -fdump-record-layouts %s -o %t.out | FileCheck %s
// REQUIRES: linux
// UNSUPPORTED: libcxx
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

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 take that line from other layout tests. I guess it is a common restriction, but I have not verified by myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understood, #2731 disables all ABI tests with libcxx.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, that's quite old one and Nick had to fix the build since then. I think that PR disabled the tests because those classes were using std::something as members, which is not the case for CompileTimeKernelInfoTy. However, if other classes don't run, then there is little value in testing this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't investigate anything but in general the libcxx abi is totally different than libstdc++ (like the names of all the internal data structures), so we would need to write a seperate test for libcxx, but just disabling it is fine since the libstdc++ version should catch any problems.

Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

@aelovikov-intel aelovikov-intel merged commit e716358 into intel:sycl Sep 29, 2025
48 of 50 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.

4 participants