[PATCH] Sema: Accept pointers to any address space for builtin functions

[David Chisnall]

Much better approach, a few comments inline.


REPOSITORY
  rL LLVM

================
Comment at: lib/Sema/SemaExpr.cpp:4583
@@ +4582,3 @@
+
+  std::vector<QualType> OverloadParams;
+  const FunctionProtoType *FT = dyn_cast<FunctionProtoType>(DeclType);
----------------
Why not a SmallVector?  I don't think this will ever have more than 4 entries (or is it 6 for the atomic compare and exchange?).  A SmallVector<QualType, 8> should avoid any heap allocation.

================
Comment at: lib/Sema/SemaExpr.cpp:4589
@@ +4588,3 @@
+  bool NeedsNewDecl = false;
+  for (unsigned i = 0, e = FT->getNumParams(); i != e; ++i) {
+
----------------
Why not use FT->param_types()?

================
Comment at: lib/Sema/SemaExpr.cpp:4611
@@ +4610,3 @@
+
+  if (!NeedsNewDecl)
+    return nullptr;
----------------
This check looks expensive.  Have you profiled this on large codebases with a lot of builtin calls?  I'd imagine that adding a DenseMap cache would be faster.

================
Comment at: lib/Sema/SemaExpr.cpp:4748
@@ +4747,3 @@
+    FunctionDecl *FDecl = dyn_cast<FunctionDecl>(NDecl);
+    if (FDecl && FDecl->getBuiltinID()) {
+      // Rewrite the function decl for this builtin by replacing paramaters
----------------
Is it worth having a fast path for builtins that don't have any pointer params?

================
Comment at: lib/Sema/SemaExpr.cpp:4393
@@ +4392,3 @@
+
+        Expr *PtrArg = DefaultFunctionArrayLvalueConversion(Arg).get();
+        QualType PtrTy = PtrArg->getType();
----------------
Anastasia wrote:
> Anastasia wrote:
> > tstellarAMD wrote:
> > > Anastasia wrote:
> > > > Not sure if this cast might create a problem in some OpenCL-GPU architectures, because spec generally disallows conversion between constant and any other address spaces (see OpenCL C v2.0 s6.5.5). 
> > > > 
> > > > I feel like a better way to handle this would be to create separate builtins overloads for constant and generic address space in OpenCL v2.0 and for all address spaces in OpenCL <v2.0. But this seems more work to me.
> > > I'm not sure about the cast issue.  I do see that for memcpy addrspacecast IR instructions are emitted.
> > > 
> > > I'm open to adding separate overloads, I just wanted to try a generic solution first.
> > Could you please look into this. The solution doesn't seem generic to me, because in OpenCL conversions from constant to other address spaces are not allowed. Which means this would most likely be a problem to handle correctly on the later compiler steps. In Clang however, we can avoid this issue by creating different overloads for such builtins and calling directly the right overload instead of having one overload and adding conversions to match it. 
> not sure how to align this with C, but for OpenCL code we would need something like:
> 
>  if address space of ptr not given, create multiple signatures of the corresponding builtin.
> 
> For example for this builtin:
> 
> BUILTIN(builtin_test, "vcC*" , "nc"), Clang would have to add the following to the list of known declarations in CL2.0:
> 
> void builtin_test(generic const char* ptr);
> void builtin_test(constant const char* ptr);
> 
> and for all earlier OpenCL versions:
> 
> void builtin_test(local const char* ptr);
> void builtin_test(global const char* ptr);
> void builtin_test(private const char* ptr);
> void builtin_test(constant const char* ptr);
> 
> However, your fix might be acceptable for C.
We need something similar for C.  We're already using (in an out-of-tree target) the specified-address-space notation for some things that only apply in one address space, but this causes some issues.  For example, we want all of the atomics builtins to support pointers in multiple address spaces, but will need to emit very different machine code (and therefore, different IR) for atomics in different address spaces.  

Address space casts have well-defined semantics on our architecture and are not always permitted for arbitrary pointers, so just casting to a different AS would work.

http://reviews.llvm.org/D8082

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/