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

[wangdi4]

When properties like alignment is specified in a annotated_ptr type, certain operators (like [], +=, ++) are disabled. This results in loop code to be written as follows:

annotated_ptr<int, decltype(properties{...alignment<8>...})> ann_ptr;
...
int *p = ann_ptr.get();     // ann_ptr cannot be used in the for loop directly
for (int i = 0; i < n; i++) {
    p[i] = i;
}

When getting the underlying pointer, the annotation gets lost, so does the the possible optimization on the for-loop brought by the annotated_ptr properties.

This PR includes changes on spec, header and clang compiler:

1. In annotated_ptr spec, update the spec for the get() function
2. In the annotated_ptr header, update the get() function by inserting llvm.ptr.annotation, so that on the target machines like FPGA for which clang FE only performs O0 optimization, the annotation inserted can be preserved for the corresponding backends to perform platform-specific optimizations. For the example above, the alignment information can help the FPGA compiler to build aligned loads/stores.
3. In the clang compiler, the pass CompileTimePropertiesPass used to always drop alignment from the annotation string. This PR changes this behavior to dropping alignment only when the compiler finds load/store/MemIntrinsics in the users of llvm.ptr.annotation and applies the alignment to these instructions.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[rolandschulz]

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

[Pennycook]

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.

[rolandschulz]

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 :).

[Pennycook]

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! 🤣

[rolandschulz]

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.

[rolandschulz]

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.

[Pennycook]

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.

[GarveyJoe]

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.

[GarveyJoe]

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.

[wangdi4]

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.

[wangdi4]

@rolandschulz @Pennycook the spec for T annotated_ptr::get() function has been updated as:

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

As for the other concern about which part of the following code should be UB:

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

, there are three options from strong to weak:

1. Initializing the annotated_ptr with unaligned pointer is UB, which is the current definition in sycl_ext_oneapi_kernel_arg_properties.asciidoc.
2. Calling any member function of the annotated pointer is UB.
3. Any access via the annotated pointer is UB. The constructor and get() aren't UB.

Both option 1 and 2 are easy for implementations to embed the assertions for the alignment. And as Joe has commented above, option 1 keeps the spec simple without the need to specify the behavior in every extension that supports the alignment property (including annotated_ptr, annotated_arg, etc).

Since the current specification is aligned with option 1 and has no ambiguity in its definition, I think we can unblock this PR and follow up the discussion in a new thread if you have further feedback on this.

[AerialMantis]

These changes look good to me, pinging @Naghasan to take a look as well.

[Naghasan]

LGTM

[wangdi4]

@intel/dpcpp-tools-reviewers could you help review the PR?

[gmlueck]

This raises an interesting question. Is there a guaranteed way to get a raw pointer that does not have the annotations? This might be important for an annotation that somehow changes the semantic of the program (i.e. not a "hint" annotation).

[wangdi4]

Right now there is no member function/operator that guarantees to get the underlying raw pointer with none annotation attached. A possible solution for this is to enable explict conversion from annotated_ptr<T> to T*.

Also I'm wondering if it's really needed. If user explicitly creates an annotated_ptr instance with a non-hint annotation, is it legal to get the underlying pointer without that annotation? Since not having this annotation changes the semantic, I would expect the compiler to issue a warning or even error if it detects the risk of losing the information.

[maksimsab]

We need to add tests of changes in CompileTimePropertiesPass.

[wangdi4]

We need to add tests of changes in CompileTimePropertiesPass.

@maksimsab new test case is now added in llvm/test/SYCLLowerIR/CompileTimePropertiesPass/sycl-properties-alignment.ll

[maksimsab]

Offloading part LGTM.

[wangdi4]

Hi @againull, this PR is ready for merge.

[againull]

---

Failed Tests (1):
SYCL :: ESIMD/regression/complex-lib-lin.cpp

failure has been seen here #12367 (comment), thus unrelated and fixable by pulling the latest sycl branch.