Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SYCL] Make sycl::marray trivially copyable #4885

Merged
merged 8 commits into from Nov 19, 2021
Merged

[SYCL] Make sycl::marray trivially copyable #4885

merged 8 commits into from Nov 19, 2021

Conversation

ghost
Copy link

@ghost ghost commented Nov 3, 2021 •
edited by ghost
Loading

The sycl::marray class is not trivially copyable but marked as device copyable. It makes problems with any class that contain a member with the sycl::marray type: that class will not be trivially copyable and therefore device copyable regardless the sycl::marray one is. To solve this issue, the sycl::marray class is converted into a trivially copyable one. The constexpr attribute for the copy constructor is saved so the class can still be copied in constant expressions.

@ghost
Copy link
Author

ghost commented Nov 8, 2021

/summary:run

@ghost
Copy link
Author

ghost commented Nov 9, 2021 •
edited by ghost
Loading

I'm not sure, break the changes the ABI since the class will be trivially copyable?

@ghost ghost marked this pull request as ready for review November 17, 2021 08:10
@ghost ghost self-requested a review as a code owner November 17, 2021 08:10
@ghost ghost requested a review from vladimirlaz November 17, 2021 08:10
Merge branch 'sycl' into make-marray-trivially-copyable
7363e52 
# Conflicts:
#	sycl/include/CL/sycl/types.hpp
vladimirlaz
vladimirlaz reviewed Nov 17, 2021
View reviewed changes
sycl/test/basic_tests/SYCL-2020-spec-constants.cpp Outdated
@@ -38,6 +38,8 @@ constexpr sycl::specialization_id<uint64_t> uint64_id(8);
constexpr sycl::specialization_id<float> float_id(10.0);
constexpr sycl::specialization_id<double> double_id(11.0);
constexpr sycl::marray<double, 5> ma;
constexpr sycl::marray<double, 5> mb(ma);
Copy link
Contributor

@vladimirlaz vladimirlaz Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is not clear why these lines were added to the test?
They are not used in specialization constants and this test is for specialization constants.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, I've moved the lines into marray.cpp test.

vladimirlaz
vladimirlaz previously approved these changes Nov 17, 2021
View reviewed changes
@ghost
Copy link
Author

ghost commented Nov 17, 2021

A test with a struct containing sycl::marray has been added into sycl/test/basic_tests/valid_kernel_args.cpp.

@Pennycook
Copy link
Contributor

Should we also add a regression test here? I'm thinking it would be good to check via a static_assert that:

  • marray<T> is trivially copyable for some trivially copyable T
  • marray<T> is device copyable for some device copyable T
  • marray<T> is not trivially copyable for some non-trivially copyable T
  • marray<T> is not device copyable for some non-device copyable T

@gmlueck
Copy link
Contributor

gmlueck commented Nov 17, 2021

I think these changes look good. I agree with @Pennycook about adding some more tests.

In addition, we should probably add code to conditionally define the trait is_device_copyable<marray<T>> to std::true when T is device copyable.

keryell
keryell reviewed Nov 18, 2021
View reviewed changes
sycl/include/CL/sycl/marray.hpp
MData[I] = Rhs.MData[I];
}
}
constexpr marray(const marray<Type, NumElements> &Rhs) = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

I love C++! :-)

@ghost
Copy link
Author

ghost commented Nov 18, 2021 •
edited by ghost
Loading

@gmlueck Sorry, I don't understand why we should define the trait is_device_copyable<marray<T>> to std::true when T is device copyable if I've removed that trait in the patch since it makes a conflict with our basic trait is_device_copyable = true for a trivially copyable T. If there are both traits, the compiler generates an error since there are ambiguous specializations of the is_device_copyable templates.

UPD I understood, you a talking about T that is device copyable but not trivially copyable, std::tuple<> for example. I've added such trait, thank you.

[SYCL] Mark sycl::marray<T> as device copyable if T is device copyable
4dab8ad 
sycl::marray is device copyable if element type is device copyable and
it is also not trivially copyable (if the element type is trivially
copyable, the marray is device copyable by default).

The tests are also added.
@ghost
Copy link
Author

ghost commented Nov 18, 2021

@Pennycook Thank you for the suggestion. I've added the tests in sycl\test\basic_tests\marray\marray.cpp.

@ghost
Copy link
Author

ghost commented Nov 18, 2021

marray<tuple<>, ...> is a bad example, since it considered trivially copyable on Linux with libstdc++ and not considered on Windows with MSVC.

@dm-vodopyanov dm-vodopyanov merged commit b5b6673 into intel:sycl Nov 19, 2021
iclsrc pushed a commit that referenced this pull request Feb 13, 2024
@aeubanks
[clang] Remove #undef alloca workaround (#81534)
742a06f 
Added in 26670dc to workaround #4885.

Windows CI and a local Windows build are happy with this change, so it
seems like this has been properly fixed at some point. If this does
break somebody, this can be easily reverted. (Also, Linux does the same
`#define alloca` in system headers, so I'm not sure why it'd be
different on Windows)

This is tech debt that caused breakages, see comments on #71709.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrodman jbrodman Awaiting requested review from jbrodman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Pennycook @gmlueck @keryell @vladimirlaz @dm-vodopyanov