[sycl] [clang] Add sycl global var attribute

[jtmott-intel]

Normally global variables are disallowed within kernels, but the presence of
this new sycl_global_var attribute will cause sema to allow that particular
global variable.

[s-kanaev]

Could this attribute be wrapped into a predefined macro (e.g. __SYCL_GLOBAL_VAR__) so, that DPCPP headers will contain the following as guard for the case when attribute isn't supported?

#ifndef __SYCL_GLOBAL_VAR__
#define __SYCL_GLOBAL_VAR__
#endif

[bader]

Is there a spec for this extension?
I'm not sure I understand the motivation for this and how this feature is supposed to work.

[s-kanaev]

This is to allow for mutable global variable, required for assert feature. See #3461

[premanandrao]

A couple of nits.

[AaronBallman]

Is this something we expect users to know when to sprinkle in their own code? I'm rather skeptical of these "trust me, I know what I'm doing" kind of attributes that take code which is otherwise ill-formed and make it behave as though it's well-formed.

[AaronBallman]

I would have expected a diagnostic here as a local variable is not a global variable.

[premanandrao]

I would have expected a diagnostic here as a local variable is not a global variable.

This is an interesting question. SYCL spec applies the "global variable" restriction (that it must be const) to variables of static storage duration.
Here: https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:language.restrictions.kernels
and here: https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace

Given this, I wonder if the attribute could be applied to the file-scope static variables to override the restriction.

[jtmott-intel]

I updated the tests and added the anonymous static union.

The anonymous union is not considered a global (doesn't accept the attribute) and isn't allowed in a kernel.

The static local var, surprisingly, is considered a global and accepts the attribute, though still not allowed in a kernel (because I didn't use the attribute's presence to allow that specific case).

These are interesting results. In my changes, I relied on the pre-defined "GlobalVar" subject list. "GlobalVar" is defined earlier as a Var where the code check S->hasGlobalStorage() is true.

I'm stepping through code so I can answer why some of these results "have global storage" and some don't.

[jtmott-intel]

Ultimately, though, this isn't a feature we're expecting external users to use. This is meant to be used by us to enable the assert feature. Perhaps there's a different subject list I can use that applies to fewer things?

[AaronBallman]

  /// Returns true for all variables that do not have local storage.
  ///
  /// This includes all global variables as well as static variables declared
  /// within a function.
  bool hasGlobalStorage() const { return !hasLocalStorage(); }

That's why you get the behavior you're getting (and the name GlobalVar in Attr.td is awful, IMHO).

Ultimately, though, this isn't a feature we're expecting external users to use. This is meant to be used by us to enable the assert feature. Perhaps there's a different subject list I can use that applies to fewer things?

It doesn't much matter what we expect -- once we expose the attribute, users will use the attribute. This is basically giving users a blessed way to opt-in to using global variables whenever they'd like (I'm not convinced this is a good idea, btw) because we found a use case where we want to do it ourselves. We can give this attribute whatever semantics we'd like, so there may be a more restrictive predicate we can use, but without a design document for the feature as such, I don't really know what the right answer is as a reviewer (I can't go off the SYCL 2020 spec because this attribute is intended specifically to do things the SYCL spec doesn't allow).

[s-kanaev]

Sounds good.

[jtmott-intel]

Latest commit expands the test to distinguish between user code and system-include code. Currently, the attribute is still allowed to appear in user code, but it won't have any effect. The attribute will only have an effect if it's used from a system header. To disallow the attribute from even appearing in user code, I'll need to figure out how to do a source location check from within a tablegen subject list.

[s-kanaev]

On the second thought, here is another example:

// sycl.hpp, system header
template <typename T>
struct Struct {
  __attribute__((sycl_global_var)) static T Variable;
};

// main.cpp, user's application
struct UserTy {
  int X;
  int Y;
};

Struct<UserTy> Global;

Is this use-case allowed?

[AaronBallman]

I would imagine that use case should be allowed -- the declaration of the variable is appropriately marked, the fact that the type comes from a user instantiation shouldn't matter to the attribute's semantics.

[jtmott-intel]

Latest commit updated with non-simple handler. Also included templates in tests. Struct<UserTy>::StaticMember would work, but Struct<UserTy> Global; would not because you can't use a global in the kernel, and you can't apply the attribute to a global in the user's cpp file (only system headers).

[keryell]

This is a useful feature for us for our HLS back-end instead of our OpenCL-ish SPIR-df back-end.
Generally useful to keep some global state like implementing memory allocation on device, having stateful random generator, etc. without requiring to pass some handlers all over the place.
Also this helps porting code from CUDA/HIP using this feature to SYCL.
It is also a feature available with OpenCL C++ 2.2 where the tricky part was how to handle non-trivial object creation and destruction, requiring kernels to run the constructors and the destructors for an OpenCL cl_program. Question: do you handle types with such constructors or destructors?
Also, what is the life-time of these global variables? Kernel? Kernel bundle? Full application?
But globally I do not really see why this extension should be restricted yo system header.

[AaronBallman]

This is a useful feature for us for our HLS back-end instead of our OpenCL-ish SPIR-df back-end.
Generally useful to keep some global state like implementing memory allocation on device, having stateful random generator, etc. without requiring to pass some handlers all over the place.
Also this helps porting code from CUDA/HIP using this feature to SYCL.
It is also a feature available with OpenCL C++ 2.2 where the tricky part was how to handle non-trivial object creation and destruction, requiring kernels to run the constructors and the destructors for an OpenCL cl_program. Question: do you handle types with such constructors or destructors?
Also, what is the life-time of these global variables? Kernel? Kernel bundle? Full application?

Good questions!

But globally I do not really see why this extension should be restricted yo system header.

I view this attribute as being akin to -fpermissive in terms of its functionality -- the SYCL spec very explicitly says "no global variables allowed" and this attribute is "haha, just kidding, allow them anyway because I said so". If the attribute is limited to only being used within the SYCL headers, then I can hold my nose and say "ah, implementation detail, pay no mind", but once it's allowed in user code, this now becomes a language feature rather than an implementation detail and that raises the bar considerably.

[AaronBallman]

I think we decided that GlobalVar was the wrong predicate (it's looking at storage only and we care about declaration context as well) and what we really want is a new kind of subject rather than checking part of the subject in Attr.td and part of it in SemaDeclAttr.cpp.

The SYCL spec hasn't really illuminated me yet, but as far as I understand the idea behind the restriction is that it restricts access based on storage (so a local static variable is disallowed because that has global storage) and something I'm not quite clear on but is either scope or linkage. Here are some code examples to demonstrate what I think should work and give errors (please correct me where I'm wrong), assuming all of this code is within a system header:

#define ATTR __attribute__((sycl_global_var))

ATTR int i; // ok
ATTR static int j; // ok
ATTR extern int k; // ok

namespace N {
ATTR int i; // ok
}

namespace {
ATTR int a; // ok
}

struct S {
  ATTR int i; // error
  ATTR static int j; // ok
};

void func(ATTR int i /* error */) {
  ATTR int l; // error
  ATTR static int m; // error

  ATTR extern int n; // error
  ATTR static constexpr int o = 0; // error

  if (ATTR static int p = 0; p == 0) // error
    ;
}

Assuming I've not gotten anything wrong, then I think the predicate we want is hasGlobalStorage() && !isLocalVarDeclOrParm(). The first allows it to be applied to a static data member of a class while the second prevents a static local variable.

Paging @erichkeane for a sanity check.

[AaronBallman]

Now that this is only usable in system headers, this can go back to being undocumented with a comment about it only being used internally by the SYCL implementation (and the docs can be removed from AttrDocs.td).

[erichkeane]

I think we decided that GlobalVar was the wrong predicate (it's looking at storage only and we care about declaration context as well) and what we really want is a new kind of subject rather than checking part of the subject in Attr.td and part of it in SemaDeclAttr.cpp.

The SYCL spec hasn't really illuminated me yet, but as far as I understand the idea behind the restriction is that it restricts access based on storage (so a local static variable is disallowed because that has global storage) and something I'm not quite clear on but is either scope or linkage. Here are some code examples to demonstrate what I think should work and give errors (please correct me where I'm wrong), assuming all of this code is within a system header:

#define ATTR __attribute__((sycl_global_var))

ATTR int i; // ok
ATTR static int j; // ok
ATTR extern int k; // ok

namespace N {
ATTR int i; // ok
}

namespace {
ATTR int a; // ok
}

struct S {
  ATTR int i; // error
  ATTR static int j; // ok
};

void func(ATTR int i /* error */) {
  ATTR int l; // error
  ATTR static int m; // error

  ATTR extern int n; // error
  ATTR static constexpr int o = 0; // error

  if (ATTR static int p = 0; p == 0) // error
    ;
}

Assuming I've not gotten anything wrong, then I think the predicate we want is hasGlobalStorage() && !isLocalVarDeclOrParm(). The first allows it to be applied to a static data member of a class while the second prevents a static local variable.

Paging @erichkeane for a sanity check.

The SYCL restriction is because global storage is not something we can properly move across the host/device barrier I think. I think the global-storage restriction for this attribute + the local-variable-or-parm restrction makes sense.

[AaronBallman]

Paging @erichkeane for a sanity check.

The SYCL restriction is because global storage is not something we can properly move across the host/device barrier I think. I think the global-storage restriction for this attribute + the local-variable-or-parm restrction makes sense.

Thanks for double-checking!

@jtmott-intel -- in this case, I think you should add a new subject to Attr.td:

def ??? : SubsetSubject<Var,
                             [{S->hasGlobalStorage() && !S->isLocalVarDeclOrParm()}], "global variables">;

I'm struggling to figure out what a good name is for ??? though. :-D How about GlobalStorageNonLocalVar to be stupidly explicit?

[jtmott-intel]

@AaronBallman

I think we decided that GlobalVar was the wrong predicate

The GlobalVar's only predicate is S->hasGlobalStorage(), which is something we wanted to check for anyway. To add extra predicates on top of that, like !S->isLocalVarDecl(), I originally used a SubsetSubject, but as the predicates we wanted to apply grew more complicated (system headers), we switched to a non-simple handler. Defining a new SubsetSubject doesn't seem like a common thing to do, and since we needed a SemaDeclAttr handler either way for the system header check, I thought it made sense to put the other extra predicates there as well. Is it still our preference to define and use a new SubsetSubject?

[AaronBallman]

LGTM, thank you!

[jtmott-intel]

I'm not authorized to merge. Could someone perform a squash merge?

[AaronBallman]

I'm not authorized to merge. Could someone perform a squash merge?

FWIW, @bader often presses the magic button automatically once the appropriate reviewers have signed off on it.