Skip to content

[SYCL] Forbid non-trivially-copyable type in annotated_ptr/ref class #11798

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 30 commits into from
Nov 29, 2023

Conversation

broxigarchen
Copy link
Contributor

@broxigarchen broxigarchen commented Nov 6, 2023

  1. Explicitly state that use non-trivially-copyable type in annotated_ptr and annotated_ref is not supported and error out if annotated_ptr is constructed with non-trivially-copyable type

  2. void is allowed on annotated_ptr as an exception

  3. Use pass-by-value in annotated_ref operator= now, and updated the spec
    this fix volatile type not supported in the following code

void func(const T& a) { ... }
volatile T b;
func(b); // this will error out

@broxigarchen broxigarchen requested a review from a team as a code owner November 6, 2023 19:11
@broxigarchen broxigarchen changed the title add support for volatile type [SYCL] Add a overload of operator=for volatile type in annotated_ref class Nov 6, 2023
@broxigarchen broxigarchen changed the title [SYCL] Add a overload of operator=for volatile type in annotated_ref class [SYCL] Add a overload of operator= for volatile type in annotated_ref class Nov 6, 2023
@broxigarchen broxigarchen requested a review from a team as a code owner November 8, 2023 21:40
@gmlueck
Copy link
Contributor

gmlueck commented Nov 14, 2023

I assume we would to the same thing here [compound assignment]

Not just that. We should do this consistently for all arguments taking const T&

Yes, I agree. However, I think the only other place there is an argument of type const T& is in the compound assignment operators.

annotated_ref doesn't own the data (like e.g. span or ptr). The assignment changes the data not what we refer to. Therefore it is correct to assign to a const annotated_ref (which is allowed by it not being const)

In this case, shouldn't we add const to all these operators for consistency?

 T operator++();
 T operator++(int);
 T operator--();
 T operator--(int);

It seems inconsistent that aptr[0] += 1 works for a const aptr but aptr[0]++ does not.

@rolandschulz
Copy link
Contributor

Yes, I agree. However, I think the only other place there is an argument of type const T& is in the compound assignment operators.

You're right.

In this case, shouldn't we add const to all these operators for consistency?

Yes. That's a bug introduced when we added those.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Runtime changes LGTM. I'll let the other's approve the extensions changes.

@broxigarchen
Copy link
Contributor Author

@intel/llvm-gatekeepers Hi can you please help to merge this? Thanks!

The CI error seems not related

@aelovikov-intel
Copy link
Contributor

Failed Tests (1):
  SYCL :: Regression/vec_rel_swizzle_ops.cpp

is already under investigation as per #12029 (comment).

I think I've seen some discussion about

Timed Out Tests (1):
  SYCL :: Basic/vector/int-convert.cpp

as well.

@aelovikov-intel aelovikov-intel merged commit 0351926 into intel:sycl Nov 29, 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.

5 participants