[SYCL][FPGA] Add clang support for buffer_location property

[MrSidims]

This is a compiler-time known accessor property which serves as an
optimization hint for a compiler on where exactly buffer was allocated.
This is needed when a board has multiple disjoint global memories that
must be managed explicitly by a programmer.

When the property is added as a template parameter of an accessor -
SemaSYCL will implicitly add an attribute to an OpenCL kernel parameters,
which were generated from SYCL kernel object. It is not allowed to use the
attribute explicitly in SYCL code.

When the attribute is applied, clang generates metadata attached to OpenCL
kernel. Number of values stored in the metadata is the same as number of kernel
parameters. Order of metadata values is following the order of pointer
kernel parameters. Metadata values are of an integer type and is being set
accordingly values passed through accessor property buffer_location.
This values are mapped in hardware backend to the actual locations of buffers
(DDR, QDR etc). Default value passed in the metadata is '-1'.

Signed-off-by: Dmitry Sidorov dmitry.sidorov@intel.com

[erichkeane]

I'm not a big fan of this approach. The info you really care about is at the parameter level.

First, don't name attributes we don't expect our users to spell.

Second, are we sure this isn't something we can calculate during codegen directly, rather than putting it in sema?

Third, if we cannot, than this is better expressed in the AST as an attribute on the ParmVarDecl instead. So each parameter of the BufferLocation type should have an attribute with the integer value attached to it (again, ONLY if we are sure we cannot figure this out during codegen).

Finally, this patch seems to make assumptions about the layout of the library. We have to be more defensive about this.

We don't necessarily control our the library (a 3rd party could write a version of the library!), nor do we have any guarantee that the version necessarily matches our release. We need to diagnose to protect our assumptions.

[MrSidims]

@erichkeane thanks for the early feedback !

Second, are we sure this isn't something we can calculate during codegen directly, rather than putting it in sema?

I think it's too late to do in CodeGen since we are constructing OpenCL kernel in Sema. In CodeGen we already see kernel declarations having pointer parameters instead of accessor objects, so no original information stored in the appropriate QualType can be queried. Also we can't even distinguish accessor pointers from USM pointers in CodeGen! I agree, that this attribute is looking nasty, but I believe for this we shall move OpenCL kernel generation out of Sema to actually CodeGen (where it belongs).

Third, if we cannot, than this is better expressed in the AST as an attribute on the ParmVarDecl instead. So each parameter of the BufferLocation type should have an attribute with the integer value attached to it (again, ONLY if we are sure we cannot figure this out during codegen).

I will think about it.

Finally, this patch seems to make assumptions about the layout of the library. We have to be more defensive about this.

We don't necessarily control our the library (a 3rd party could write a version of the library!), nor do we have any guarantee that the version necessarily matches our release. We need to diagnose to protect our assumptions.

Thanks! I'll add some (warnings?) diagnostics then.

[erichkeane]

They'd likely need to be errors, and stop execution. If the buffer isn't the right layout it seems that it should just be an ill-formed program.

[MrSidims]

They'd likely need to be errors, and stop execution. If the buffer isn't the right layout it seems that it should just be an ill-formed program.

I disagree. As for now accessor properties are Intel extension and serves only for optimization purposes (semantically they have no affect, but metadata generation). Hence if some one wants to use Intel compiler and a third-party library I see no reasons to deny them this opportunity (emitting warning and stop whatever compiler was up to do with this property is good enough).

[erichkeane]

They'd likely need to be errors, and stop execution. If the buffer isn't the right layout it seems that it should just be an ill-formed program.

I disagree. As for now accessor properties are Intel extension and serves only for optimization purposes (semantically they have no affect, but metadata generation). Hence if some one wants to use Intel compiler and a third-party library I see no reasons to deny them this opportunity (emitting warning and stop whatever compiler was up to do with this property is good enough).

In that case, you absolutely cannot make any assumptions on the layout of them. The purpose of errors is to be an assertion type-thing, where we would require this buffer_location property to be a certain format. If we don't emit those as errors, NO assumptions on them being valid can be made. I'm alright with this, but it means you have to be much more careful with the implementation.

[MrSidims]

They'd likely need to be errors, and stop execution. If the buffer isn't the right layout it seems that it should just be an ill-formed program.

I disagree. As for now accessor properties are Intel extension and serves only for optimization purposes (semantically they have no affect, but metadata generation). Hence if some one wants to use Intel compiler and a third-party library I see no reasons to deny them this opportunity (emitting warning and stop whatever compiler was up to do with this property is good enough).

In that case, you absolutely cannot make any assumptions on the layout of them. The purpose of errors is to be an assertion type-thing, where we would require this buffer_location property to be a certain format. If we don't emit those as errors, NO assumptions on them being valid can be made. I'm alright with this, but it means you have to be much more careful with the implementation.

Sure, thanks! For now this implementation doesn't break any SYCL tests, even we have no support for this feature in the headers, so I assume I was quite careful :)

[erichkeane]

They'd likely need to be errors, and stop execution. If the buffer isn't the right layout it seems that it should just be an ill-formed program.

I disagree. As for now accessor properties are Intel extension and serves only for optimization purposes (semantically they have no affect, but metadata generation). Hence if some one wants to use Intel compiler and a third-party library I see no reasons to deny them this opportunity (emitting warning and stop whatever compiler was up to do with this property is good enough).

In that case, you absolutely cannot make any assumptions on the layout of them. The purpose of errors is to be an assertion type-thing, where we would require this buffer_location property to be a certain format. If we don't emit those as errors, NO assumptions on them being valid can be made. I'm alright with this, but it means you have to be much more careful with the implementation.

Sure, thanks! For now this implementation doesn't break any SYCL tests, even we have no support for this feature in the headers, so I assume I was quite careful :)

You weren't particularly, I see a number of assumptions you make, like that the template arguments are types, etc. I pointed out a few, but everything you pull from the buffer_location property isn't something that can be counted on.

[Fznamznon]

Any spec links for this one?
Seems like it is exposed to user.

[MrSidims]

What have changed:
0. I figured out, that my previous implementation version is incorrect: instead of passing buffer_location property to accessor's template parameter, users will pass property_list there, so the implementation was adjusted.

1. The attribute now is spellingless, also it's being applied to ParmVarDecl instead of Function declaration;
2. Lowered number of assumptions (or at least I think so), added several diagnostics. @erichkeane some of these diagnostics remains untested, because current implementation of sycl.hpp disallows on CPP level some use cases, when they can fire. I can also add something like fake-sycl.hpp with other accessor/property_list possible implementations to test them, what do you think?

@Fznamznon spec for compiletime known properties can be found here: #1925 . For buffer location itself only SPIR-V spec is public now: #2084 , ping @mohammadfawaz and @GarveyJoe to make SYCL spec public.

[MrSidims]

Addressed the comments, major:

• No more '-1' in SemaSYCL, yet I slightly lean towards having it as a default MD value;
• handleBufferLocationProperty was split into handleAccessorPropertyList and handleBufferLocationProperty

I left @Fznamznon comment #2166 (comment) unaddressed, since I don't really know, how to improve special type detection

[MrSidims]

Seems like CPU RT in our CI isn't ready for this change SPIR-V-wise, so I have reverted SPIR-V support of the feature for testing.

[MrSidims]

This patch needs to be merged first: #2240

[Fznamznon]

Looks ok, but please get approve from @erichkeane first.

[erichkeane]

See a few missed diagnostics.

[erichkeane]

This seems correct now, thanks.