[PATCH] D111566: [SYCL] Fix function pointer address space

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.
Herald added a project: All.



Comment at: clang/lib/AST/ASTContext.cpp:11579
+unsigned ASTContext::getTargetAddressSpace(QualType T) const {
+  return T->isFunctionType() ? getTargetInfo().getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());

Can we add a DataLayout aware function to CodeGen instead? That would avoid the 
need for getProgramAddressSpace() in TargetInfo?



Comment at: clang/lib/Basic/TargetInfo.cpp:153
   MaxOpenCLWorkGroupSize = 1024;
+  ProgramAddrSpace = 0;
 }

A bit late to this review (only just noticed it while merging) - but I don't 
like that we end up duplicating even more DataLayout information here - while 
it only affects AVR upstream, downstream CHERI and Morello have to duplicate 
this to Arm/RISC-V/MIPS as well now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4eaf5846d0e7: [clang] Fix function pointer address space 
(authored by eandrews).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AVR.h
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGen/avr/functionptr-addrspace.c
  clang/test/CodeGenSYCL/functionptr-addrspace.cpp

Index: clang/test/CodeGenSYCL/functionptr-addrspace.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/functionptr-addrspace.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -triple spir64 -verify -emit-llvm %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func ) {
+  kernelFunc();
+}
+
+// CHECK: define dso_local spir_func{{.*}}invoke_function{{.*}}(i32 ()* %fptr, i32 addrspace(4)* %ptr)
+void invoke_function(int (*fptr)(), int *ptr) {}
+
+int f() { return 0; }
+
+int main() {
+  kernel_single_task([=]() {
+int (*p)() = f;
+int ()() = *p;
+int a = 10;
+invoke_function(p, );
+invoke_function(r, );
+invoke_function(f, );
+  });
+  return 0;
+}
Index: clang/test/CodeGen/avr/functionptr-addrspace.c
===
--- /dev/null
+++ clang/test/CodeGen/avr/functionptr-addrspace.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm %s -o - | FileCheck %s
+
+int main() {
+  int (*p)();
+  return 0;
+}
+
+// CHECK: %p = alloca i16 (...) addrspace(1)*
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -643,11 +643,7 @@
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
 if (PointeeType->isVoidTy())
   PointeeType = llvm::Type::getInt8Ty(getLLVMContext());
-
-unsigned AS = PointeeType->isFunctionTy()
-  ? getDataLayout().getProgramAddressSpace()
-  : Context.getTargetAddressSpace(ETy);
-
+unsigned AS = Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
@@ -748,7 +744,13 @@
 llvm::Type *PointeeType = CGM.getLangOpts().OpenCL
   ? CGM.getGenericBlockLiteralType()
   : ConvertTypeForMem(FTy);
-unsigned AS = Context.getTargetAddressSpace(FTy);
+// Block pointers lower to function type. For function type,
+// getTargetAddressSpace() returns default address space for
+// function pointer i.e. program address space. Therefore, for block
+// pointers, it is important to pass qualifiers when calling
+// getTargetAddressSpace(), to ensure that we get the address space
+// for data pointers and not function pointers.
+unsigned AS = Context.getTargetAddressSpace(FTy.getQualifiers());
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
Index: clang/lib/Basic/Targets/AVR.h
===
--- clang/lib/Basic/Targets/AVR.h
+++ clang/lib/Basic/Targets/AVR.h
@@ -55,6 +55,7 @@
 Int16Type = SignedInt;
 Char32Type = UnsignedLong;
 SigAtomicType = SignedChar;
+ProgramAddrSpace = 1;
 resetDataLayout("e-P1-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8");
   }
 
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -150,6 +150,7 @@
   PlatformMinVersion = VersionTuple();
 
   MaxOpenCLWorkGroupSize = 1024;
+  ProgramAddrSpace = 0;
 }
 
 // Out of line virtual dtor for TargetInfo.
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11575,6 +11575,15 @@
   return getTargetInfo().getNullPointerValue(AS);
 }
 
+unsigned ASTContext::getTargetAddressSpace(QualType T) const {
+  return T->isFunctionType() ? getTargetInfo().getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}
+
+unsigned ASTContext::getTargetAddressSpace(Qualifiers Q) const {
+  return getTargetAddressSpace(Q.getAddressSpace());
+}
+
 unsigned ASTContext::getTargetAddressSpace(LangAS AS) const {
   if (isTargetAddressSpace(AS))
 return toTargetAddressSpace(AS);
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ 

[PATCH] D111566: [SYCL] Fix function pointer address space

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

LGTM, thanks!




Comment at: clang/test/CodeGen/avr/functionptr-addrspace.c:3
+
+int main() {
+  int (*p)();

eandrews wrote:
> When writing the test I noticed an existing crash in the compiler. I am not 
> sure if I am doing something wrong but if you try to assign to the pointer, 
> you will hit the following assert - 
> https://llvm.org/doxygen/Constants_8cpp_source.html#l02280
> 
> ```
> int f() { return 0; }
> 
> int main() {
>   int (*p)() = f;
>   return 0;
> }
> ```
> 
> I briefly debugged this and I think the crash is because in 
> `GetAddrOfFunction` we call `ConvertType` which returns type without the 
> address space here - 
> https://clang.llvm.org/doxygen/CodeGenTypes_8cpp_source.html#l00419.
> 
> I didn't fix this or debug this further since it is outside the scope of this 
> patch but I can look into it as a followup PR
Okay.  AVR may be a constrained enough environment that they don't actually run 
into this sort of thing, but yeah, it'd be great to chase these bugs down.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added inline comments.



Comment at: clang/test/CodeGen/avr/functionptr-addrspace.c:3
+
+int main() {
+  int (*p)();

When writing the test I noticed an existing crash in the compiler. I am not 
sure if I am doing something wrong but if you try to assign to the pointer, you 
will hit the following assert - 
https://llvm.org/doxygen/Constants_8cpp_source.html#l02280

```
int f() { return 0; }

int main() {
  int (*p)() = f;
  return 0;
}
```

I briefly debugged this and I think the crash is because in `GetAddrOfFunction` 
we call `ConvertType` which returns type without the address space here - 
https://clang.llvm.org/doxygen/CodeGenTypes_8cpp_source.html#l00419.

I didn't fix this or debug this further since it is outside the scope of this 
patch but I can look into it as a followup PR


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 399484.
eandrews added a comment.
Herald added a subscriber: Jim.

Implemented review comments

- Remove unnecessary set method and set default value during construction 
instead
- Changed default address space for function pointer for avr target to 1.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AVR.h
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGen/avr/functionptr-addrspace.c
  clang/test/CodeGenSYCL/functionptr-addrspace.cpp

Index: clang/test/CodeGenSYCL/functionptr-addrspace.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/functionptr-addrspace.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -triple spir64 -verify -emit-llvm %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func ) {
+  kernelFunc();
+}
+
+// CHECK: define dso_local spir_func{{.*}}invoke_function{{.*}}(i32 ()* %fptr, i32 addrspace(4)* %ptr)
+void invoke_function(int (*fptr)(), int *ptr) {}
+
+int f() { return 0; }
+
+int main() {
+  kernel_single_task([=]() {
+int (*p)() = f;
+int ()() = *p;
+int a = 10;
+invoke_function(p, );
+invoke_function(r, );
+invoke_function(f, );
+  });
+  return 0;
+}
Index: clang/test/CodeGen/avr/functionptr-addrspace.c
===
--- /dev/null
+++ clang/test/CodeGen/avr/functionptr-addrspace.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm %s -o - | FileCheck %s
+
+int main() {
+  int (*p)();
+  return 0;
+}
+
+// CHECK: %p = alloca i16 (...) addrspace(1)*
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -643,11 +643,7 @@
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
 if (PointeeType->isVoidTy())
   PointeeType = llvm::Type::getInt8Ty(getLLVMContext());
-
-unsigned AS = PointeeType->isFunctionTy()
-  ? getDataLayout().getProgramAddressSpace()
-  : Context.getTargetAddressSpace(ETy);
-
+unsigned AS = Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
@@ -748,7 +744,13 @@
 llvm::Type *PointeeType = CGM.getLangOpts().OpenCL
   ? CGM.getGenericBlockLiteralType()
   : ConvertTypeForMem(FTy);
-unsigned AS = Context.getTargetAddressSpace(FTy);
+// Block pointers lower to function type. For function type,
+// getTargetAddressSpace() returns default address space for
+// function pointer i.e. program address space. Therefore, for block
+// pointers, it is important to pass qualifiers when calling
+// getTargetAddressSpace(), to ensure that we get the address space
+// for data pointers and not function pointers.
+unsigned AS = Context.getTargetAddressSpace(FTy.getQualifiers());
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
Index: clang/lib/Basic/Targets/AVR.h
===
--- clang/lib/Basic/Targets/AVR.h
+++ clang/lib/Basic/Targets/AVR.h
@@ -55,6 +55,7 @@
 Int16Type = SignedInt;
 Char32Type = UnsignedLong;
 SigAtomicType = SignedChar;
+ProgramAddrSpace = 1;
 resetDataLayout("e-P1-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8");
   }
 
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -148,6 +148,7 @@
   PlatformMinVersion = VersionTuple();
 
   MaxOpenCLWorkGroupSize = 1024;
+  ProgramAddrSpace = 0;
 }
 
 // Out of line virtual dtor for TargetInfo.
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11488,6 +11488,15 @@
   return getTargetInfo().getNullPointerValue(AS);
 }
 
+unsigned ASTContext::getTargetAddressSpace(QualType T) const {
+  return T->isFunctionType() ? getTargetInfo().getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}
+
+unsigned ASTContext::getTargetAddressSpace(Qualifiers Q) const {
+  return getTargetAddressSpace(Q.getAddressSpace());
+}
+
 unsigned ASTContext::getTargetAddressSpace(LangAS AS) const {
   if (isTargetAddressSpace(AS))
 return toTargetAddressSpace(AS);
Index: clang/include/clang/Basic/TargetInfo.h
===
--- 

[PATCH] D111566: [SYCL] Fix function pointer address space

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:763
+  /// Set the address space for functions for the given target.
+  virtual void setProgramAddressSpace() { ProgramAddrSpace = 0; }
+

eandrews wrote:
> I'm not entirely sure about setting the default value to zero here. I set it 
> to 0 here based on default value for ProgramAddrSpace  in DataLayout class 
> (https://llvm.org/doxygen/DataLayout_8cpp_source.html#l00186)
> 
> Targets with non-zero value should override this function. I do not know what 
> these targets are (if any). I ran check-llvm and nothing failed. 
I don't think you need this method; targets that want to change the program 
address space can just set it during construction, since it's a protected 
field.  I know there are some existing places that use this virtual `set` 
pattern, but there doesn't seem to be a good reason for it.

Looks like AVR sets the program address space to 1.  (I did a regexp search for 
`/["-]P[0-9]/` in `lib/Basic/`.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews marked 3 inline comments as done.
eandrews added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:763
+  /// Set the address space for functions for the given target.
+  virtual void setProgramAddressSpace() { ProgramAddrSpace = 0; }
+

I'm not entirely sure about setting the default value to zero here. I set it to 
0 here based on default value for ProgramAddrSpace  in DataLayout class 
(https://llvm.org/doxygen/DataLayout_8cpp_source.html#l00186)

Targets with non-zero value should override this function. I do not know what 
these targets are (if any). I ran check-llvm and nothing failed. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 399080.
eandrews added a comment.

Add program address space to TargetInfo. Targets with non-default address space 
for functions should explicitly set value.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/Targets.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGenSYCL/functionptr-addressspace.cpp

Index: clang/test/CodeGenSYCL/functionptr-addressspace.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/functionptr-addressspace.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -triple spir64 -verify -emit-llvm %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func ) {
+  kernelFunc();
+}
+
+// CHECK: define dso_local spir_func{{.*}}invoke_function{{.*}}(i32 ()* %fptr, i32 addrspace(4)* %ptr)
+void invoke_function(int (*fptr)(), int *ptr) {}
+
+int f() { return 0; }
+
+int main() {
+  kernel_single_task([=]() {
+int (*p)() = f;
+int ()() = *p;
+int a = 10;
+invoke_function(p, );
+invoke_function(r, );
+invoke_function(f, );
+  });
+  return 0;
+}
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -643,11 +643,7 @@
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
 if (PointeeType->isVoidTy())
   PointeeType = llvm::Type::getInt8Ty(getLLVMContext());
-
-unsigned AS = PointeeType->isFunctionTy()
-  ? getDataLayout().getProgramAddressSpace()
-  : Context.getTargetAddressSpace(ETy);
-
+unsigned AS = Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
@@ -748,7 +744,13 @@
 llvm::Type *PointeeType = CGM.getLangOpts().OpenCL
   ? CGM.getGenericBlockLiteralType()
   : ConvertTypeForMem(FTy);
-unsigned AS = Context.getTargetAddressSpace(FTy);
+// Block pointers lower to function type. For function type,
+// getTargetAddressSpace() returns default address space for
+// function pointer i.e. program address space. Therefore, for block
+// pointers, it is important to pass qualifiers when calling
+// getTargetAddressSpace(), to ensure that we get the address space
+// for data pointers and not function pointers.
+unsigned AS = Context.getTargetAddressSpace(FTy.getQualifiers());
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
Index: clang/lib/Basic/Targets.cpp
===
--- clang/lib/Basic/Targets.cpp
+++ clang/lib/Basic/Targets.cpp
@@ -718,6 +718,7 @@
   Target->setSupportedOpenCLOpts();
   Target->setCommandLineOpenCLOpts();
   Target->setMaxAtomicWidth();
+  Target->setProgramAddressSpace();
 
   if (!Target->validateTarget(Diags))
 return nullptr;
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11488,6 +11488,15 @@
   return getTargetInfo().getNullPointerValue(AS);
 }
 
+unsigned ASTContext::getTargetAddressSpace(QualType T) const {
+  return T->isFunctionType() ? getTargetInfo().getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}
+
+unsigned ASTContext::getTargetAddressSpace(Qualifiers Q) const {
+  return getTargetAddressSpace(Q.getAddressSpace());
+}
+
 unsigned ASTContext::getTargetAddressSpace(LangAS AS) const {
   if (isTargetAddressSpace(AS))
 return toTargetAddressSpace(AS);
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -213,6 +213,7 @@
   unsigned char RegParmMax, SSERegParmMax;
   TargetCXXABI TheCXXABI;
   const LangASMap *AddrSpaceMap;
+  unsigned ProgramAddrSpace;
 
   mutable StringRef PlatformName;
   mutable VersionTuple PlatformMinVersion;
@@ -756,6 +757,11 @@
 return getTypeWidth(IntMaxType);
   }
 
+  unsigned getProgramAddressSpace() const { return ProgramAddrSpace; }
+
+  /// Set the address space for functions for the given target.
+  virtual void setProgramAddressSpace() { ProgramAddrSpace = 0; }
+
   // Return the size of unwind_word for this target.
   virtual unsigned getUnwindWordWidth() const { return getPointerWidth(0); }
 
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ 

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews marked 2 inline comments as done.
eandrews added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:11496
+   .getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}

rjmccall wrote:
> Oh, I'm sorry I missed this.  Parsing the data layout string into an 
> `llvm::DataLayout` is definitely not an okay thing to be doing here.  The 
> IRGen version of this had a cached `DataLayout` object which it queried, 
> which was okay, but this function is used too tightly to be doing that much 
> redundant work.
> 
> We could just cache a `DataLayout` in the `clang::TargetInfo`, but I think 
> we've been trying to avoid that as a layering violation.  Instead, 
> `TargetInfo` should just have a `getProgramAddressSpace` method or something 
> like that, and the targets with non-default address spaces for code should 
> set that manually.
> Instead, `TargetInfo` should just have a `getProgramAddressSpace` method or 
> something like that, and the targets with non-default address spaces for code 
> should set that manually.

Ok. Thanks for review! I'll make this change



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:11496
+   .getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}

Oh, I'm sorry I missed this.  Parsing the data layout string into an 
`llvm::DataLayout` is definitely not an okay thing to be doing here.  The IRGen 
version of this had a cached `DataLayout` object which it queried, which was 
okay, but this function is used too tightly to be doing that much redundant 
work.

We could just cache a `DataLayout` in the `clang::TargetInfo`, but I think 
we've been trying to avoid that as a layering violation.  Instead, `TargetInfo` 
should just have a `getProgramAddressSpace` method or something like that, and 
the targets with non-default address spaces for code should set that manually.



Comment at: clang/lib/AST/ASTContext.cpp:11497
+   .getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}

eandrews wrote:
> rjmccall wrote:
> > If a function type has an address space qualifier, we should prefer that, 
> > right?  Or is that impossible by construction?
> I thought you could only use address space qualifiers for variables. I am not 
> sure though. 
> 
> This patch retains existing behavior for pointers.  The existing code 
> (deleted in CodeGenTypes.cpp in his patch) doesn't take qualifiers into 
> consideration. 
Ah, yes, I see.  Alright.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 398784.
eandrews added a comment.

Implement review comments - add a comment + remove unnecessary code


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

Files:
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGenSYCL/functionptr-addressspace.cpp

Index: clang/test/CodeGenSYCL/functionptr-addressspace.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/functionptr-addressspace.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -triple spir64 -verify -emit-llvm %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func ) {
+  kernelFunc();
+}
+
+// CHECK: define dso_local spir_func{{.*}}invoke_function{{.*}}(i32 ()* %fptr, i32 addrspace(4)* %ptr)
+void invoke_function(int (*fptr)(), int *ptr) {}
+
+int f() { return 0; }
+
+int main() {
+  kernel_single_task([=]() {
+int (*p)() = f;
+int ()() = *p;
+int a = 10;
+invoke_function(p, );
+invoke_function(r, );
+invoke_function(f, );
+  });
+  return 0;
+}
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -643,11 +643,7 @@
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
 if (PointeeType->isVoidTy())
   PointeeType = llvm::Type::getInt8Ty(getLLVMContext());
-
-unsigned AS = PointeeType->isFunctionTy()
-  ? getDataLayout().getProgramAddressSpace()
-  : Context.getTargetAddressSpace(ETy);
-
+unsigned AS = Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
@@ -748,7 +744,13 @@
 llvm::Type *PointeeType = CGM.getLangOpts().OpenCL
   ? CGM.getGenericBlockLiteralType()
   : ConvertTypeForMem(FTy);
-unsigned AS = Context.getTargetAddressSpace(FTy);
+// Block pointers lower to function type. For function type,
+// getTargetAddressSpace() returns default address space for
+// function pointer i.e. program address space. Therefore, for block
+// pointers, it is important to pass qualifiers when calling
+// getTargetAddressSpace(), to ensure that we get the address space
+// for data pointers and not function pointers.
+unsigned AS = Context.getTargetAddressSpace(FTy.getQualifiers());
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -80,6 +80,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/IR/DataLayout.h"
 #include "llvm/Support/Capacity.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
@@ -11488,6 +11489,17 @@
   return getTargetInfo().getNullPointerValue(AS);
 }
 
+unsigned ASTContext::getTargetAddressSpace(QualType T) const {
+  return T->isFunctionType()
+ ? llvm::DataLayout(getTargetInfo().getDataLayoutString())
+   .getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}
+
+unsigned ASTContext::getTargetAddressSpace(Qualifiers Q) const {
+  return getTargetAddressSpace(Q.getAddressSpace());
+}
+
 unsigned ASTContext::getTargetAddressSpace(LangAS AS) const {
   if (isTargetAddressSpace(AS))
 return toTargetAddressSpace(AS);
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2709,13 +2709,9 @@
   QualType getFloatingTypeOfSizeWithinDomain(QualType typeSize,
  QualType typeDomain) const;
 
-  unsigned getTargetAddressSpace(QualType T) const {
-return getTargetAddressSpace(T.getQualifiers());
-  }
+  unsigned getTargetAddressSpace(QualType T) const;
 
-  unsigned getTargetAddressSpace(Qualifiers Q) const {
-return getTargetAddressSpace(Q.getAddressSpace());
-  }
+  unsigned getTargetAddressSpace(Qualifiers Q) const;
 
   unsigned getTargetAddressSpace(LangAS AS) const;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:11494
+  const Type *TypePtr = T.getTypePtr();
+  return TypePtr->isFunctionType()
+ ? llvm::DataLayout(getTargetInfo().getDataLayoutString())

rjmccall wrote:
> You can just do `T->isFunctionType()`.
Thanks! I'll make the change



Comment at: clang/lib/AST/ASTContext.cpp:11497
+   .getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}

rjmccall wrote:
> If a function type has an address space qualifier, we should prefer that, 
> right?  Or is that impossible by construction?
I thought you could only use address space qualifiers for variables. I am not 
sure though. 

This patch retains existing behavior for pointers.  The existing code (deleted 
in CodeGenTypes.cpp in his patch) doesn't take qualifiers into consideration. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:11494
+  const Type *TypePtr = T.getTypePtr();
+  return TypePtr->isFunctionType()
+ ? llvm::DataLayout(getTargetInfo().getDataLayoutString())

You can just do `T->isFunctionType()`.



Comment at: clang/lib/AST/ASTContext.cpp:11497
+   .getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}

If a function type has an address space qualifier, we should prefer that, 
right?  Or is that impossible by construction?



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:747
   : ConvertTypeForMem(FTy);
-unsigned AS = Context.getTargetAddressSpace(FTy);
+unsigned AS = Context.getTargetAddressSpace(FTy.getQualifiers());
 ResultType = llvm::PointerType::get(PointeeType, AS);

Please add a comment here explaining that it's important to not just use `FTy` 
because in the absence of an explicit address space we need to get the default 
address space for a data pointer, not a function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

In D111566#3228949 , @rjmccall wrote:

> Block pointers are actually data pointers and should stay in the generic 
> address space if they don't have an address space qualifier.  That might 
> require new logic.

Thanks! I kept the existing behavior for block pointers by passing qualifiers 
to `getTargetAddressSpace`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 398723.
eandrews added a comment.

Implemented review comment to move logic into function 
(`getTargetAddressSpace`) as opposed to call site.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

Files:
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGenSYCL/functionptr-addressspace.cpp

Index: clang/test/CodeGenSYCL/functionptr-addressspace.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/functionptr-addressspace.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -triple spir64 -verify -emit-llvm %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func ) {
+  kernelFunc();
+}
+
+// CHECK: define dso_local spir_func{{.*}}invoke_function{{.*}}(i32 ()* %fptr, i32 addrspace(4)* %ptr)
+void invoke_function(int (*fptr)(), int *ptr) {}
+
+int f() { return 0; }
+
+int main() {
+  kernel_single_task([=]() {
+int (*p)() = f;
+int ()() = *p;
+int a = 10;
+invoke_function(p, );
+invoke_function(r, );
+invoke_function(f, );
+  });
+  return 0;
+}
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -643,11 +643,7 @@
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
 if (PointeeType->isVoidTy())
   PointeeType = llvm::Type::getInt8Ty(getLLVMContext());
-
-unsigned AS = PointeeType->isFunctionTy()
-  ? getDataLayout().getProgramAddressSpace()
-  : Context.getTargetAddressSpace(ETy);
-
+unsigned AS = Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
@@ -748,7 +744,7 @@
 llvm::Type *PointeeType = CGM.getLangOpts().OpenCL
   ? CGM.getGenericBlockLiteralType()
   : ConvertTypeForMem(FTy);
-unsigned AS = Context.getTargetAddressSpace(FTy);
+unsigned AS = Context.getTargetAddressSpace(FTy.getQualifiers());
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -80,6 +80,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/IR/DataLayout.h"
 #include "llvm/Support/Capacity.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
@@ -11488,6 +11489,18 @@
   return getTargetInfo().getNullPointerValue(AS);
 }
 
+unsigned ASTContext::getTargetAddressSpace(QualType T) const {
+  const Type *TypePtr = T.getTypePtr();
+  return TypePtr->isFunctionType()
+ ? llvm::DataLayout(getTargetInfo().getDataLayoutString())
+   .getProgramAddressSpace()
+ : getTargetAddressSpace(T.getQualifiers());
+}
+
+unsigned ASTContext::getTargetAddressSpace(Qualifiers Q) const {
+  return getTargetAddressSpace(Q.getAddressSpace());
+}
+
 unsigned ASTContext::getTargetAddressSpace(LangAS AS) const {
   if (isTargetAddressSpace(AS))
 return toTargetAddressSpace(AS);
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2709,13 +2709,9 @@
   QualType getFloatingTypeOfSizeWithinDomain(QualType typeSize,
  QualType typeDomain) const;
 
-  unsigned getTargetAddressSpace(QualType T) const {
-return getTargetAddressSpace(T.getQualifiers());
-  }
+  unsigned getTargetAddressSpace(QualType T) const;
 
-  unsigned getTargetAddressSpace(Qualifiers Q) const {
-return getTargetAddressSpace(Q.getAddressSpace());
-  }
+  unsigned getTargetAddressSpace(Qualifiers Q) const;
 
   unsigned getTargetAddressSpace(LangAS AS) const;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Block pointers are actually data pointers and should stay in the generic 
address space.  I... don't honestly know why they get lowered as function 
pointers; they probably shouldn't be.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);

rjmccall wrote:
> aaron.ballman wrote:
> > eandrews wrote:
> > > aaron.ballman wrote:
> > > > The review summary says that this is a fix for SYCL, but the fix itself 
> > > > happens for all targets, not just SYCL. If that's intentional, are we 
> > > > sure it's correct?
> > > Yes this affects all targets. To be honest, I'm not sure if this change 
> > > is correct for CUDA/openCL, etc. My first patch (which I didn't upload) 
> > > restricted the change to SYCL. However, I saw the same thing was done in 
> > > a generic manner for function pointers  - 
> > > https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d,
> > >  and so followed the same logic. I'm hoping reviewers more familiar with 
> > > address spaces can help here. 
> > @Anastasia -- can you comment as OpenCL code owner?
> I think the more systematic fix is probably for `getTargetAddressSpace` to 
> check for function types and return the program address space, yeah.
@rjmccall @dylanmckay I took a look into this today. Moving the code to 
`getTargetAddressSpace ` causes a few openCL tests to fail because address 
space changes for block pointers.

```
 case Type::BlockPointer: {
 const QualType FTy = cast(Ty)->getPointeeType();
 llvm::Type *PointeeType = CGM.getLangOpts().OpenCL
   ? CGM.getGenericBlockLiteralType()
   : ConvertTypeForMem(FTy);
 unsigned AS = Context.getTargetAddressSpace(FTy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
```
Here PointeeType (FTy) is a function type and so 
Context.getTargetAddressSpace(FTy) will return ProgramAddressSpace after my 
change. 

IR Change  - 


```
void foo(){
int (^ block_B)(void) = ^{
return i;
  };
  block_B();
}

OLDIR   %block_B = alloca %struct.__opencl_block_literal_generic addrspace(4)*, 
align 4
---
NEWIR   %block_B = alloca %struct.__opencl_block_literal_generic*, align 4
```

Would you know if this is correct behavior for block pointers? Or is the 
existing behavior correct here?

I apologize for the delay in responding to your review. I did not get a chance 
to work on this before I left for vacation last month.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

Thanks for the reviews @dylanmckay and @rjmccall ! I agree that moving the 
logic for functions pointers to `getTargetAddressSpace`  makes sense. However, 
I'm not sure what the consequences are, since that increases the impact of this 
change quite a bit. I'm not sure if I will have the time to deal with any 
issues that arise before I go on vacation for Christmas. I'll take a quick look 
sometime this week, and hopefully its a simple enough change. If not, I can do 
it in a follow-up PR as suggested above, unless someone objects.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);

aaron.ballman wrote:
> eandrews wrote:
> > aaron.ballman wrote:
> > > The review summary says that this is a fix for SYCL, but the fix itself 
> > > happens for all targets, not just SYCL. If that's intentional, are we 
> > > sure it's correct?
> > Yes this affects all targets. To be honest, I'm not sure if this change is 
> > correct for CUDA/openCL, etc. My first patch (which I didn't upload) 
> > restricted the change to SYCL. However, I saw the same thing was done in a 
> > generic manner for function pointers  - 
> > https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d,
> >  and so followed the same logic. I'm hoping reviewers more familiar with 
> > address spaces can help here. 
> @Anastasia -- can you comment as OpenCL code owner?
I think the more systematic fix is probably for `getTargetAddressSpace` to 
check for function types and return the program address space, yeah.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Dylan McKay via Phabricator via cfe-commits]

dylanmckay accepted this revision.
dylanmckay added a comment.

By the way, as this has already been approved by one, and you rightly applied 
the "speak now or forever hold your peace" principle re. OpenCL, and this 
clearly works better from my point of view than the old code, I wouldn't want 
to prevent you from committing this. **I have no objections to merging this in 
its current state**. If you do merge it as-is though it would be nice if a 
follow up PR moves the logic into `getTargetAddressSpace()` where I think it 
better belongs


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Dylan McKay via Phabricator via cfe-commits]

dylanmckay added a comment.

This will product correct code from an AVR perspective where (before this patch 
it would have failed codegen). It is consistent with how we construct function 
pointers in LLVM core as well.

I'm not very familiar with Clang internals, one thing that stick out to me:

**The logic instead belongs directly inside the existing 
`getTargetAddressSpace` method?**

Perhaps the new `if (is function) { set address space to the value from the 
data layout}` logic belongs directly inside `ASTContext::getTargetAddressSpace` 
or one of its descendants in the call tree. This would obviously increase the 
scope/possible impact of the change further. The reason I suspect it belongs in 
there is that in theory, if this is the correct way to get the address space 
for a `QualType` (which may be a function) **in this instance**, then I feel 
that same logic would hold for any other `QualType` that may be a function 
pointer that has `getTargetAddressSpace()` called on it because I don't see 
anything special or unique about this existing call to `getTargetAddressSpace` 
versus any other existing call to it inside clang. If you implement it at that 
lower level inside `getTargetAddressSpace`, your conditional would be something 
like `QualType.getTypePtr()->isFunctionType()` etc.

This patch fixes one callsite of  `getTargetAddressSpace` but there are several 
other existing callsites remaining which if called with a function, they *would 
not* return the appropriate address space.

If someone more familar with clang than me disagrees please chime in


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

ping * 3

Since this patch has been accepted by one code owner, and has been under review 
for over a month, I plan to submit it by the end of the week. If anyone has 
feedback/concerns, please comment on the review.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);

eandrews wrote:
> aaron.ballman wrote:
> > The review summary says that this is a fix for SYCL, but the fix itself 
> > happens for all targets, not just SYCL. If that's intentional, are we sure 
> > it's correct?
> Yes this affects all targets. To be honest, I'm not sure if this change is 
> correct for CUDA/openCL, etc. My first patch (which I didn't upload) 
> restricted the change to SYCL. However, I saw the same thing was done in a 
> generic manner for function pointers  - 
> https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d,
>  and so followed the same logic. I'm hoping reviewers more familiar with 
> address spaces can help here. 
@Anastasia -- can you comment as OpenCL code owner?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);

aaron.ballman wrote:
> The review summary says that this is a fix for SYCL, but the fix itself 
> happens for all targets, not just SYCL. If that's intentional, are we sure 
> it's correct?
Yes this affects all targets. To be honest, I'm not sure if this change is 
correct for CUDA/openCL, etc. My first patch (which I didn't upload) restricted 
the change to SYCL. However, I saw the same thing was done in a generic manner 
for function pointers  - 
https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d,
 and so followed the same logic. I'm hoping reviewers more familiar with 
address spaces can help here. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: rjmccall, erichkeane, Anastasia.
aaron.ballman added a comment.

Adding a few more reviewers with more familiarity with codegen and address 
spaces to make sure we've not missed something here outside of SYCL.




Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);

The review summary says that this is a fix for SYCL, but the fix itself happens 
for all targets, not just SYCL. If that's intentional, are we sure it's correct?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

@vlastik, your commit fixes function pointers on AVR - 
https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d.
 I suppose this change is required for fixing lvalue references to function 
pointers on AVR as well. Right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 379562.
eandrews added a comment.

Added a test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

Files:
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGenSYCL/functionptr-addressspace.cpp


Index: clang/test/CodeGenSYCL/functionptr-addressspace.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/functionptr-addressspace.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -triple spir64 -verify 
-emit-llvm %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func ) {
+  kernelFunc();
+}
+
+// CHECK: define dso_local spir_func{{.*}}invoke_function{{.*}}(i32 ()* %fptr, 
i32 addrspace(4)* %ptr)
+void invoke_function(int (*fptr)(), int *ptr) {}
+
+int f() { return 0; }
+
+int main() {
+  kernel_single_task([=]() {
+int (*p)() = f;
+int ()() = *p;
+int a = 10;
+invoke_function(p, );
+invoke_function(r, );
+invoke_function(f, );
+  });
+  return 0;
+}
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -633,7 +633,9 @@
 const ReferenceType *RTy = cast(Ty);
 QualType ETy = RTy->getPointeeType();
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
-unsigned AS = Context.getTargetAddressSpace(ETy);
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }


Index: clang/test/CodeGenSYCL/functionptr-addressspace.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/functionptr-addressspace.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -triple spir64 -verify -emit-llvm %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func ) {
+  kernelFunc();
+}
+
+// CHECK: define dso_local spir_func{{.*}}invoke_function{{.*}}(i32 ()* %fptr, i32 addrspace(4)* %ptr)
+void invoke_function(int (*fptr)(), int *ptr) {}
+
+int f() { return 0; }
+
+int main() {
+  kernel_single_task([=]() {
+int (*p)() = f;
+int ()() = *p;
+int a = 10;
+invoke_function(p, );
+invoke_function(r, );
+invoke_function(f, );
+  });
+  return 0;
+}
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -633,7 +633,9 @@
 const ReferenceType *RTy = cast(Ty);
 QualType ETy = RTy->getPointeeType();
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
-unsigned AS = Context.getTargetAddressSpace(ETy);
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

need a test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111566: [SYCL] Fix function pointer address space

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews created this revision.
eandrews added reviewers: vlastik, dylanmckay, bader.
Herald added subscribers: Naghasan, Anastasia, ebevhan, yaxunl.
eandrews requested review of this revision.

Functions pointers should be created with program address space. This patch 
fixes a crash on lvalue reference to function pointer (in device code) when 
using oneAPI DPC++ compiler.


https://reviews.llvm.org/D111566

Files:
  clang/lib/CodeGen/CodeGenTypes.cpp


Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -633,7 +633,9 @@
 const ReferenceType *RTy = cast(Ty);
 QualType ETy = RTy->getPointeeType();
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
-unsigned AS = Context.getTargetAddressSpace(ETy);
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }


Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -633,7 +633,9 @@
 const ReferenceType *RTy = cast(Ty);
 QualType ETy = RTy->getPointeeType();
 llvm::Type *PointeeType = ConvertTypeForMem(ETy);
-unsigned AS = Context.getTargetAddressSpace(ETy);
+unsigned AS = PointeeType->isFunctionTy()
+  ? getDataLayout().getProgramAddressSpace()
+  : Context.getTargetAddressSpace(ETy);
 ResultType = llvm::PointerType::get(PointeeType, AS);
 break;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits