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.

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

Turn new address space rules by default #242

Merged
merged 4 commits into from Jun 25, 2019
Merged

Conversation

asavonic
Copy link
Contributor

This patch enables new rules for address space handling in SYCL by
default. Previous rules for address space handling can still be
enabled by DISABLE_INFER_AS=1 environment variable.

This approach is known as "generic pointers by default", and
it essentially makes all pointers without an explicit address space
qualifier to be pointers in generic address space.

For example:

void foo() {
  int* ip = foo();
}

void* bar(int* ip);

template <typename T>
void swap(T&, T&) {}

All pointers (and references) in the example above are CodeGen'ed as
pointers in addrspace(4) in LLVM IR (where '4' is a number for generic
address space).

Concrete address space (global, local and private) can originate from:

  1. an address-of operation applied to an automatic variable:
int i = 42;
(void) &i // <--- this is a private pointer
  1. Static variable

  2. SYCL global, local, or constant buffers

In all three cases, once this pointer is used, it is immediately
addrspacecast'ed to generic, because a user does not (and should not)
specify address space for pointers in source code.

Andrew Savonichev added 4 commits June 21, 2019 20:24
Static variables without address space now reside in global address
space, unless they have an explicit address space qualifier in source
code.

Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
We cannot return a generic pointer like in 'global', 'local' and
'private' cases, because 'constant' and 'generic' address spaces do
not overlap.

This patch returns __constant pointer as is, and assumes that a user
have to deal with __constant pointer limitations (ie. such pointers
cannot be casted to plain (default) pointers).

Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
This patch enables new rules for address space handling in SYCL by
default. Previous rules for address space handling can still be
enabled by DISABLE_INFER_AS=1 environment variable.

This approach is known as "generic pointers by default", and
it essentially makes all pointers without an explicit address space
qualifier to be pointers in generic address space.

For example:

    void foo() {
      int* ip = foo();
    }

    void* bar(int* ip);

    template <typename T>
    void swap(T&, T&) {}

All pointers (and references) in the example above are CodeGen'ed as
pointers in addrspace(4) in LLVM IR (where '4' is a number for generic
address space).

Concrete address space (global, local and private) can originate from:

  1) an address-of operation applied to an automatic variable:
     int i = 42;
     (void) &i // <--- this is a private pointer

  2) Static variable

  3) SYCL global, local, or constant buffers

In all three cases, once this pointer is used, it is immediately
addrspacecast'ed to generic, because a user does not (and should not)
specify address space for pointers in source code.

Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
Although OpenCL specification explicitly states that a string literal
should reside in constant address space, it does not work for SYCL
with "generic by default" address space rules.

For example:

   const char *getLiteral() {
     return "A";
   }

   void func(bool AorB) {
     char B[] = {'B', '\0'};
     const char* C = AorB ? A : B;
   }

If `A' reside in constant address space, it cannot be returned from a
function `getLiteral', because it returns a generic const char*.

Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
@romanovvlad romanovvlad self-requested a review June 24, 2019 12:48
@Ruyk
Copy link
Contributor

Ruyk commented Jun 24, 2019

previous rules for address space handling can still be
enabled by DISABLE_INFER_AS=1 environment variable.

Could this be implemented via a command-line flag instead of an environmental variable?

@asavonic
Copy link
Contributor Author

previous rules for address space handling can still be
enabled by DISABLE_INFER_AS=1 environment variable.

Could this be implemented via a command-line flag instead of an environmental variable?

I considered making an option for this, but it takes more effort to pass a flag to all places this patch changes, and it doesn't seem to worth it. DISABLE_INFER_AS=1 variable should be treated as a fallback solution until all regressions introduced by this feature are found and fixed. After that, DISABLE_INFER_AS variable is going to be removed.

Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

I am unsure to understand the consequences of this.
Is there a phase later, such as an LLVM pass. to recover what could be the good address-space in an OpenCL 1.x world?
Or are you in a SYCL 2.2 mood and thus you plan to target only OpenCL 2.y devices?

@@ -63,7 +63,7 @@ class LLVM_LIBRARY_VISIBILITY SPIRTargetInfo : public TargetInfo {
VLASupported = false;
LongWidth = LongAlign = 64;
if (Triple.getEnvironment() == llvm::Triple::SYCLDevice &&
getenv("ENABLE_INFER_AS")) {
!getenv("DISABLE_INFER_AS")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if it is a temporary solution, naming the environment variable such as SYCL_CLANG_DISABLE_INFER_AS or whatever so we can understand among the crowded output of a printenv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change the variable name in a follow-up commit.

@@ -1063,6 +1063,8 @@ static void InitializePredefinedMacros(const TargetInfo &TI,
// SYCL device compiler which doesn't produce host binary.
if (LangOpts.SYCLIsDevice) {
Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
if (!getenv("DISABLE_INFER_AS"))
Builder.defineMacro("__SYCL_ENABLE_INFER_AS__", "1");
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this name should serve as a starting point for the environment variable.

@asavonic asavonic assigned asavonic and romanovvlad and unassigned asavonic Jun 25, 2019
@asavonic
Copy link
Contributor Author

I am unsure to understand the consequences of this.
Is there a phase later, such as an LLVM pass. to recover what could be the good address-space in an OpenCL 1.x world?

InferAddressSpaces can be used to cleanup generic address space pointers in the optimization phase. It generally performs pretty good on an optimized IR (after mem2reg and inlining is done).

Or are you in a SYCL 2.2 mood and thus you plan to target only OpenCL 2.y devices?

If a device does not support OpenCL 2.0 Generic address space feature, then such device can be used only if you run InferAddressSpaces pass (or a similar pass) before passing code to the device compiler. In some cases, generic address space pointers cannot be resolved statically, and this approach will not work.

OpenCL 2.0 (or 1.x + GAS feature) devices should work fine with any SYCL code.

clang/lib/CodeGen/CodeGenModule.cpp Show resolved Hide resolved
// const char *getLiteral() n{
// return "AB";
// }
return LangAS::opencl_private;
Copy link
Contributor

Choose a reason for hiding this comment

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

    // If we keep a literal string in constant address space, the following code
    // becomes illegal:

Why not global then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question - I don't know what is the best address space for this.
I used private address space mainly because it was used to lower a string literal before I changed address spaces by this patch.

@romanovvlad romanovvlad merged commit c4e6b09 into intel:sycl Jun 25, 2019
vladimirlaz pushed a commit that referenced this pull request Oct 28, 2020
  CONFLICT (modify/delete): clang/test/CodeGenSYCL/unique-stable-name.cpp deleted in cec49a5 and modified in HEAD. Version HEAD of clang/test/CodeGenSYCL/unique-stable-name.cpp left in tree.
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.

None yet

4 participants