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] Insert annotation in annotated_ptr::get() #12343

Merged
merged 10 commits into from
Feb 20, 2024
Merged

[SYCL] Insert annotation in annotated_ptr::get() #12343

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e35dd53
insert annotation in annotated_ptr::get()
wangdi4 Jan 10, 2024
efb363e
fix comment & test failure
wangdi4 Jan 11, 2024
246b1ef
Merge remote-tracking branch 'origin/sycl' into attach_annotation_to_get
wangdi4 Jan 11, 2024
0457819
Merge remote-tracking branch 'origin/sycl' into attach_annotation_to_get
wangdi4 Jan 12, 2024
88fe720
code refactor
wangdi4 Jan 15, 2024
3ebd695
Merge commit '8959c3cf987f94f8514d30fc82923c662088cad3' into attach_a…
wangdi4 Jan 22, 2024
dfbaad2
add lit & update spec
wangdi4 Jan 31, 2024
4b2b904
fix lit
wangdi4 Feb 1, 2024
a0597c8
add test case for CompileTimePropertiesPass
wangdi4 Feb 16, 2024
4b63406
Merge branch 'sycl' of https://github.com/intel/llvm into attach_anno…
wangdi4 Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion sycl/include/sycl/ext/oneapi/experimental/annotated_ptr/annotated_ptr.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,15 @@ __SYCL_TYPE(annotated_ptr) annotated_ptr<T, detail::properties_t<Props...>> {

operator T *() const noexcept = delete;

T *get() const noexcept { return m_Ptr; }
inline T *get() const noexcept {
wangdi4 marked this conversation as resolved.
Show resolved Hide resolved
#ifdef __SYCL_DEVICE_ONLY__
return __builtin_intel_sycl_ptr_annotation(
Copy link
Contributor

Choose a reason for hiding this comment

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

@Pennycook & @gmlueck : Currently the spec says "The raw pointer will not retain the annotations." Should this be changed to something like "It is implementation defined whether the raw pointer retains any annotations."?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we precisely define anywhere what an "annotation" is?

I think it's important that get() returns a raw pointer that behaves exactly the same way as a T*, and that any annotations that remain are not reflected in the type system. I'm not sure, but my guess is that because we say this returns a T*, that must already be true?

If you agree, I'm not opposed to saying that the raw pointer may retain some annotations. But if the user has no way of checking whether the annotations are there, or what an annotation even is, it seems like we could also achieve something similar by saying something vague like "Implementations are free to propagate information from properties of an annotated_ptr to the raw pointer." That may also be simpler than implementation-defined, because an implementation may not be able to succinctly describe all the situations in which the annotations will exist and continue to work.

Copy link
Contributor

@rolandschulz rolandschulz Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's important that get() returns a raw pointer that behaves exactly the same way as a T*, and that any annotations that remain are not reflected in the type system. I'm not sure, but my guess is that because we say this returns a T*, that must already be true?

Yes I agree this needs to be clear. For me it is clear because it says it returns T*. Happy to make this clearer if you think it isn't clear.

"Implementations are free to propagate information from properties of an annotated_ptr to the raw pointer."

I'm OK with that too. Another option might be:

"An implementation can use the information of the properties for optimizations of access to the raw pointer."

Do we agree that if the user does an invalid access according to a property (e.g. deference an unaligned pointer with the alignment property), that this is UB even if the access is through the raw pointer obtained from get()? If so is this clear enough with either of those wordings? Or do we need to explicitly state that any UB access is still UB for the raw pointer obtained from get?

That may also be simpler than implementation-defined, because an implementation may not be able to succinctly describe all the situations in which the annotations will exist and continue to work.

For open-source compilers "implementation defined" often just means: the compiler code defines the behaviour :).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree this needs to be clear. For me it is clear because it says it returns T*. Happy to make this clearer if you think it isn't clear.

If you think it's clear, I'm happy with it as is. I just wanted to make sure we were on the same page.

"An implementation can use the information of the properties for optimizations of access to the raw pointer."

I'm okay with that too.

Do we agree that if the user does an invalid access according to a property (e.g. deference an unaligned pointer with the alignment property), that this is UB even if the access is through the raw pointer obtained from get()?

I don't think I agree with this, but I'm trying to think of exactly what this looks like in code.

annotated_ptr<float> ptr = /* some aligned pointer */;
float* raw = ptr.get();

raw[1] = ...; // are you suggesting this should be UB?

raw++;
*raw = ...; // are you suggesting this should be UB?

I think the code above should not be UB. I think the implementation should be free to keep the annotations in place as an optimization, but only if doing so doesn't impact correctness.

I don't think that's inconsistent with the way things are defined, because in the example above a compiler would be smart enough to know that although raw is aligned, raw[1] and the result of raw++ are not. The implementation would be free to continue using aligned loads/stores for any access via raw that it can prove is aligned, but shouldn't assume alignment for all accesses.

This is specific to alignment, of course. There might be cases where things really do end up being UB (like casting away a read-only or write-only property). I'm not sure how to bucket things or how to document that bucketing.

For open-source compilers "implementation defined" often just means: the compiler code defines the behaviour :).

Brilliant! 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

annotated_ptr<float> ptr = /* some aligned pointer */;
float* raw = ptr.get();

raw[1] = ...; // are you suggesting this should be UB?

raw++;
*raw = ...; // are you suggesting this should be UB?

No. I agree that this isn't UB because the property doesn't apply after you modify it.

An example to show my concern:

annotated_ptr<float, properties(alignment<...>)> ptr = /* some *un-*aligned pointer */;
float* raw = ptr.get();
*raw = ...;

This should be UB. I think the current wording isn't clear which line is UB. I think it would be good if the first two lines aren't UB. We have seen with nullptr being passed as kernel argument that users don't like if unused things are UB. And if assignment/get are UB in this example the example is UB even if the pointer is unused (with the 3rd line removed). But if we say that assignment and get are never UB, than we should clarify that pointer access is UB even if done through the raw pointer.

Copy link
Contributor

Choose a reason for hiding this comment

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

To make this a bit more concrete: Should an implementation be allowed to put an assert for the property (e.g. alignment) in the constructor/assignment and/or get, or only be allowed to assert the property when used (e.g. operator*)? Note that assert (which kills the program) is only allowed if an operation is UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. Thanks for the example.

I agree that your example should be UB, and that it isn't clear from the specification exactly which part makes it UB.

I'd be okay with either:

  1. Using the wording you proposed (i.e., any access via the annotated pointer is UB). The constructor and get() aren't UB.

  2. Using slightly stronger wording (e.g., calling any member function of the annotated pointer is UB). Then the constructor wouldn't be UB, but calling get() would be UB.

One small advantage of 2. over 1. is that an implementation could use an assert as you suggested to throw an error. That's much easier than having to throw an error when the raw pointer is dereferenced.

Copy link
Contributor

@GarveyJoe GarveyJoe Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would strongly prefer we keep the first line UB:

annotated_ptr<float, properties(alignment<...>)> ptr = /* some *un-*aligned pointer */;

This keeps the spec simple. We don't have to mention anything about propagating annotations; we merely have to define the property in such a way that it gives us information about the alignment of the pointer. The propagation is a legal optimization/use of the information we're given with no need to be documented. It's also an easy model to think about: only put the property on a pointer you know is aligned.

FYI, my interpretation is consistent with how the property is defined today in this extension: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_arg_properties.asciidoc.

m_Ptr, detail::PropertyMetaInfo<Props>::name...,
detail::PropertyMetaInfo<Props>::value...);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will annotate the pointer with all its properties, right? I don't think this is generally legal; this should be property dependent. For example, if a property was worded like this then you would not be allowed to apply it in this case:

"foo: the foo property directs the compiler to ensure that all accesses through this annotated_ptr should be done using foo loads or stores."

On the other hand, if it's worded like this (like alignment) then it is legal to apply it:

"foo: the foo property indicates that this pointer has foo characteristics."

The key difference is whether the property is information that the user is telling us about the pointer or whether it's a directive from the user for us to treat all dereferences of the pointer in a specific way.

Copy link
Contributor Author

@wangdi4 wangdi4 Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The code has been updated by adding a filter for the properties in Props... predicated on the type trait propagateToPtrAnnotation. Right now only two properties (buffer_location and alignment) are added to the whitelist.

#else
return m_Ptr;
#endif
}

annotated_ptr &operator++() noexcept {
m_Ptr += 1;
Expand Down
Loading