WIP: [SYCL] Implement SYCL address space attributes handling #968

bader · 2019-12-24T14:31:43Z

This pull request is created to gather SYCL working group internal feedback before uploading the patch to LLVM phabricator.

Default address space (applies when no explicit address space was
specified) maps to generic (4) address space.

Added SYCLDevice environment to triple.

Static variables without address space now reside in global address
space, unless they have an explicit address space qualifier in source
code.

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*.

When default address space values in C++ are CodeGen'ed into different
address spaces in LLVM IR, we must ensure that an addrspacecast is
emitted before a bitcast.

Pointers with the same address space in AST may end up in different
address spaces in IR. We cannot CodeGen a conditional
operator (ternary 'if') whose operands have different address spaces,
so we have to addrspacecast them to generic.

When a static variable is lowered from AST to LLVM IR, it can change an
address space depending on its storage class. For example, a static
variable declared in function scope may be assigned to global address
space if language rules allow. When this happens, original address space
of a variable (retuned by D.getType()) is no longer can be used as
target address space. Correct address space for a global can be obtained
from CodeGenModule::GetGlobalVarAddressSpace function, but in this case
we just copy it from the existing global (OldGV).

Pointers with the same address space in AST may end up in different
address spaces in IR. We cannot CodeGen a binary compare
operator (<, >, ==, etc.) whose operands have different address spaces,
so we have to addrspacecast them to generic.

Emit address space cast if return type and return value have different
address spaces.

Invalid bitcast with different address spaces was emitted as arument of
llvm.invariant.start intrinsic for work group scope constants.
This problem caused assertion fail.

Make sure that all operands of phi instruction that is generated while
lowering conditional operator have the same type. This fixes compiler
assertion.

Default address space (applies when no explicit address space was specified) maps to generic (4) address space. Added SYCLDevice environment to triple. Static variables without address space now reside in global address space, unless they have an explicit address space qualifier in source code. 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*. When default address space values in C++ are CodeGen'ed into different address spaces in LLVM IR, we must ensure that an addrspacecast is emitted before a bitcast. Pointers with the same address space in AST may end up in different address spaces in IR. We cannot CodeGen a conditional operator (ternary 'if') whose operands have different address spaces, so we have to addrspacecast them to generic. When a static variable is lowered from AST to LLVM IR, it can change an address space depending on its storage class. For example, a static variable declared in function scope may be assigned to global address space if language rules allow. When this happens, original address space of a variable (retuned by D.getType()) is no longer can be used as target address space. Correct address space for a global can be obtained from CodeGenModule::GetGlobalVarAddressSpace function, but in this case we just copy it from the existing global (OldGV). Pointers with the same address space in AST may end up in different address spaces in IR. We cannot CodeGen a binary compare operator (<, >, ==, etc.) whose operands have different address spaces, so we have to addrspacecast them to generic. Emit address space cast if return type and return value have different address spaces. Invalid bitcast with different address spaces was emitted as arument of llvm.invariant.start intrinsic for work group scope constants. This problem caused assertion fail. Make sure that all operands of phi instruction that is generated while lowering conditional operator have the same type. This fixes compiler assertion. Signed-off-by: Alexey Bader <alexey.bader@intel.com>

Fznamznon

I don't have any objections, just a couple of questions/suggestions.
@asavonic , it would be nice if you take a look.

Fznamznon · 2019-12-25T08:56:06Z

clang/lib/AST/ItaniumMangle.cpp

@@ -2285,7 +2285,7 @@ void CXXNameMangler::mangleQualifiers(Qualifiers Quals, const DependentAddressSp
    if (Context.getASTContext().addressSpaceMapManglingFor(AS)) {
      //  <target-addrspace> ::= "AS" <address-space-number>
      unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS);
-      if (TargetAS != 0)
+      if (TargetAS != 0 || (Context.getASTContext().getLangOpts().SYCLIsDevice))


Don't you know why we need this change?

sycl_private address is mapped to 0 for SPIR as well as default address space, so in order to "specialize" templates with the sycl_private qualified types, we have to mangle them differently from the unqualified types.
Test case is here: https://github.com/intel/llvm/pull/968/files#diff-4e74dfb66665848ae647af85422c78d8R41

sycl_private address is mapped to 0 for SPIR as well as default address space

Default address space is supposed to be akin to generic, i.e. it should be mapped to 4.

Sorry, my previous reply is not quite accurate.
This change is needed because pointers qualified with address space mapped to 0 LLVM AS are mangled the same way as unquliafied pointers.

Ok, that makes sense.
However, I'm surprised that we don't have a similar code for OpenCL here. opencl_private is also mapped to 0, but it has a distinct mangling:

__attribute__((overloadable)) void foo(__private int *ip) { *ip = 0; } // magled as: _Z3fooPU9CLprivatei

We have similar problem for OpenCL targeting SPIR. You seemed to use some other target your example. X86?

Sorry, the problem I'm talking about is mangling, not the mangled names conflict.
Mangled names conflict is not a problem in OpenCL as OpenCL compiler qualifies all the pointers.

We have similar problem for OpenCL targeting SPIR. You seemed to use some other target your example. X86?

Right, sorry for the confusion. With -triple spir the function is mangled as _Z3fooPi.

Sorry, the problem I'm talking about is mangling, not the mangled names conflict.
Mangled names conflict is not a problem in OpenCL as OpenCL compiler qualifies all the pointers.

I don't quite understand what the problem is. Can provide a test case?

The test case is added to this PR already: https://github.com/intel/llvm/pull/968/files#diff-9cbf615457fd73dff7e9840ed855fa9a and problem is covered in this thread. I just clarified why it affects only SYCL compiler, but not OpenCL.
Smaller reproducer:

template<typename T> void tmpl(T t){} __attribute__((opencl_private)) int *PRIV; tmpl(PRIV); int *NoAS; tmpl(NoAS);

SYCL compiler produces the same mangled name for two tmpl instances, OpenCL compiler adds generic qualifier for NoAS declaration and produces different names.
Let me know if it's still unclear.

I think original patch from @erichkeane mapped sycl_private address space to AS 5. This might be an alternative option, but we need LLVM-SPIRV translator to support alternative mappings as well.

clang/include/clang/Sema/ParsedAttr.h

Fznamznon · 2019-12-25T09:04:29Z

clang/include/clang/AST/Type.h

+            (other.getAddressSpace() == LangAS::sycl_private ||
+             other.getAddressSpace() == LangAS::sycl_local ||
+             other.getAddressSpace() == LangAS::sycl_global ||
+             other.getAddressSpace() == LangAS::sycl_constant));


Hmm, maybe it would be good to add a comment explaining why we allowed casting between "empty" address space and others. At least for casts to sycl_constant address spaces.

@asavonic, can you help with that?

casts to sycl_constant address spaces

I don't think this casts should be allowed as they are not legal in OpenCL env.

Another option would be skipping sycl_constant in this patch.

I don't think this casts should be allowed as they are not legal in OpenCL env.

Agree.

llvm/include/llvm/ADT/Triple.h

asavonic · 2019-12-25T20:59:05Z

clang/include/clang/Basic/AddressSpaces.h

@@ -42,6 +42,12 @@ enum class LangAS : unsigned {
  cuda_constant,
  cuda_shared,

+  // SYCL specific address spaces.


Please remind me why we define these SYCL address spaces, and not use the OpenCL ones?

I experimented in this direction and existing rules for OpenCL address spaces prohibit casts between qualified and unqualified pointers. I can share my patch if you are interested.
If I understand the logic you implemented is semantically different from the OpenCL semantics, but as long as we produce valid SPIR-V module it should be fine.

FYI: current status of experiment with using OpenCL address spaces can be found in my private fork.

clang/lib/CodeGen/CGCall.cpp

asavonic · 2019-12-25T21:21:15Z

clang/test/SemaSYCL/address-space-parameter-conversions.cpp

+  __attribute__((opencl_local)) int *LOC;
+  int *NoAS;
+
+  bar(*GLOB);


Since bar takes a private pointer (reference), and *GLOB is global pointer (reference), we essentially addrspace cast here from global to private. Why this is a positive test case?

If I understand it correctly, there is no invalid casts thanks to this overload:

void bar(int & Data) {}

+@erichkeane to confirm.

Signed-off-by: Alexey Bader <alexey.bader@intel.com>

bader · 2020-01-22T16:00:39Z

Alternative implementation: #1039. Please, take a look and let me know if you have any concerns regarding OpenCL address space attributes re-use in SYCL mode.
Once we finalize the approach, I'll update the patch for upstream to llvm.org.

bader · 2020-04-25T16:58:05Z

This approach is abandoned in favor of OpenCL attributes re-use.
See #1581.

bader added the upstream This change is related to upstreaming SYCL support to llorg. label Dec 24, 2019

bader added this to the Upstream basic SYCL device front-end compiler milestone Dec 24, 2019

bader requested review from asavonic, erichkeane and Fznamznon December 24, 2019 14:31

bader self-assigned this Dec 24, 2019

bader mentioned this pull request Dec 24, 2019

[SYCL] Implement SYCL address space attributes handling bader/llvm#4

Closed

Fznamznon reviewed Dec 25, 2019

View reviewed changes

asavonic reviewed Dec 25, 2019

View reviewed changes

bader added 2 commits January 18, 2020 14:48

Apply code review suggestion to fix spelling in the comments

c1302db

Signed-off-by: Alexey Bader <alexey.bader@intel.com>

Applied code review suggestion

29e63b5

Signed-off-by: Alexey Bader <alexey.bader@intel.com>

bader changed the title ~~[SYCL] Implement SYCL address space attributes handling~~ WIP: [SYCL] Implement SYCL address space attributes handling Jan 22, 2020

bader mentioned this pull request Jan 22, 2020

[SYCL] Re-use OpenCL address space attributes for SYCL #1039

Closed

bader closed this Apr 25, 2020

bader deleted the sycl-address-space branch April 25, 2020 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: [SYCL] Implement SYCL address space attributes handling #968

WIP: [SYCL] Implement SYCL address space attributes handling #968

bader commented Dec 24, 2019

Fznamznon left a comment

Fznamznon Dec 25, 2019

bader Dec 25, 2019

asavonic Dec 25, 2019

bader Jan 18, 2020

asavonic Jan 20, 2020

bader Jan 20, 2020

bader Jan 20, 2020

asavonic Jan 20, 2020

bader Jan 20, 2020

Fznamznon Dec 25, 2019

bader Jan 18, 2020

asavonic Jan 20, 2020

asavonic Dec 25, 2019

bader Jan 18, 2020

bader Jan 20, 2020

asavonic Dec 25, 2019

bader Jan 18, 2020

bader commented Jan 22, 2020

bader commented Apr 25, 2020

WIP: [SYCL] Implement SYCL address space attributes handling #968

WIP: [SYCL] Implement SYCL address space attributes handling #968

Conversation

bader commented Dec 24, 2019

This pull request is created to gather SYCL working group internal feedback before uploading the patch to LLVM phabricator.

Fznamznon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Jan 22, 2020

bader commented Apr 25, 2020