Sema: Accept pointers to any address space for builtin functions

[• tstellarAMD]

Summary

As long as they don't have an address space explicitly defined.

This allows builtins with pointer arguments to be used with OpenCL.

[Anastasia]

Shall TODO/comment be added somewhere saying that handling address space number of pointers in builtins is still to be implemented?

Also what the number should be for OpenCL? Should it be taken from LangAS::ID enum?

lib/Sema/SemaExpr.cpp

4393

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.

[• tstellarAMD]

In D8082#134961, @Anastasia wrote:

Shall TODO/comment be added somewhere saying that handling address space number of pointers in builtins is still to be implemented?

This appears to be implemented already, but there are no builtins using it. What does need to be done is add a way to
specify a named address space, like constant or local, because right now the builtins only accept integer address spaces.

lib/Sema/SemaExpr.cpp

4393

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.

[Anastasia]

lib/Sema/SemaExpr.cpp

4393

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.

[Anastasia]

lib/Sema/SemaExpr.cpp

4393

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.

[• tstellarAMD]

Here is an updated patch with an entirely different approach. Instead of inserting addressspace casts, this patch rewrites the function declaration to match the address space of the call arguments.

[theraven]

Much better approach, a few comments inline.

lib/Sema/SemaExpr.cpp

4393

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.

4583

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.

4589

Why not use FT->param_types()?

4611

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.

4751

Is it worth having a fast path for builtins that don't have any pointer params?

[• tstellarAMD]

lib/Sema/SemaExpr.cpp

4611

I think the fast-path for builtins without pointers would help here. What keys/data were you thinking would go in the DenseMap?

[Anastasia]

lib/Sema/SemaExpr.cpp

4615

Should address space of return type also be taken into account?

4753

typo: arguemnts -> arguments

test/Sema/builtins.cl

5

remove empty line

6

You need "expected-no-diagnostics" only once in the file (for example after RUN line).

9

remove empty line

[• tstellarAMD]

Hi, here is an updated patch. I've addressed all comments except I did not add a DensMap to rewriteBuiltinFunctionDecl(), I wasn't sure exactly how to use this to speed up the function. I can still add this if people think it will help. I did some profiling of a function with 100,000 __builtin_memcpy() calls and this patch doesn't seem to increase compile time.

I also did not add handling for return types. I wasn't able to figure out how to get the actual call expression which had the return type used in the code. I added a TODO for this.

[theraven]

Looks good to me! Thanks for working on this.

[Anastasia]

LGTM!

include/clang/Basic/Builtins.h

     return strchr(GetRecord(ID).Attributes, 't') != nullptr;
   }
 
+  /// \brief Determines whether this builtin has a result or any arguments which
+  /// are pointer types.
+  bool hasPtrArgsOrResult(unsigned ID) const {
+    return strchr(GetRecord(ID).Type, '*') != nullptr;
+  }
+
   /// \brief Completely forget that the given ID was ever considered a builtin,
   /// e.g., because the user provided a conflicting signature.
   void ForgetBuiltin(unsigned ID, IdentifierTable &Table);

include/clang/Basic/Builtins.def

 //  I   -> Required to constant fold to an integer constant expression.
 //
 // Types may be postfixed with the following modifiers:
-// * -> pointer (optionally followed by an address space number)
+// * -> pointer (optionally followed by an address space number, if no address
+//               space is specified than any address space will be accepted)
 // & -> reference (optionally followed by an address space number)
 // C -> const
 // D -> volatile

lib/Sema/SemaExpr.cpp

   return hasInvalid;
 }
 
+/// If a builtin function has a pointer argument with no explicit address
+/// space, than it should be able to accept a pointer to any address
+/// space as input.  In order to do this, we need to replace the
+/// standard builtin declaration with one that uses the same address space
+/// as the call.
+///
+/// \returns nullptr If this builtin is not a candidate for a rewrite i.e.
+///                  it does not contain any pointer arguments without
+///                  an address space qualifer.  Otherwise the rewritten
+///                  FunctionDecl is returned.
+/// TODO: Handle pointer return types.
+static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext &Context,
+                                                const FunctionDecl *FDecl,
+                                                MultiExprArg ArgExprs) {
+
+  QualType DeclType = FDecl->getType();
+  const FunctionProtoType *FT = dyn_cast<FunctionProtoType>(DeclType);
+
+  if (!Context.BuiltinInfo.hasPtrArgsOrResult(FDecl->getBuiltinID()) ||
+      !FT || FT->isVariadic() || ArgExprs.size() != FT->getNumParams())
+    return nullptr;
+
+  bool NeedsNewDecl = false;
+  unsigned i = 0;
+  SmallVector<QualType, 8> OverloadParams;
+
+  for (QualType ParamType : FT->param_types()) {
+
+    // Convert array arguments to pointer to simplify type lookup.
+    Expr *Arg = Sema->DefaultFunctionArrayLvalueConversion(ArgExprs[i++]).get();
+    QualType ArgType = Arg->getType();
+    if (!ParamType->isPointerType() ||
+        ParamType.getQualifiers().hasAddressSpace() ||
+        !ArgType->isPointerType() ||
+        !ArgType->getPointeeType().getQualifiers().hasAddressSpace()) {
+      OverloadParams.push_back(ParamType);
+      continue;
+    }
+
+    NeedsNewDecl = true;
+    unsigned AS = ArgType->getPointeeType().getQualifiers().getAddressSpace();
+
+    QualType PointeeType = ParamType->getPointeeType();
+    PointeeType = Context.getAddrSpaceQualType(PointeeType, AS);
+    OverloadParams.push_back(Context.getPointerType(PointeeType));
+  }
+
+  if (!NeedsNewDecl)
+    return nullptr;
+
+  FunctionProtoType::ExtProtoInfo EPI;
+  QualType OverloadTy = Context.getFunctionType(FT->getReturnType(),
+                                                OverloadParams, EPI);
+  DeclContext *Parent = Context.getTranslationUnitDecl();
+  FunctionDecl *OverloadDecl = FunctionDecl::Create(Context, Parent,
+                                                    FDecl->getLocation(),
+                                                    FDecl->getLocation(),
+                                                    FDecl->getIdentifier(),
+                                                    OverloadTy,
+                                                    /*TInfo=*/nullptr,
+                                                    SC_Extern, false,
+                                                    /*hasPrototype=*/true);
+  SmallVector<ParmVarDecl*, 16> Params;
+  FT = cast<FunctionProtoType>(OverloadTy);
+  for (unsigned i = 0, e = FT->getNumParams(); i != e; ++i) {
+    QualType ParamType = FT->getParamType(i);
+    ParmVarDecl *Parm =
+        ParmVarDecl::Create(Context, OverloadDecl, SourceLocation(),
+                                SourceLocation(), nullptr, ParamType,
+                                /*TInfo=*/nullptr, SC_None, nullptr);
+    Parm->setScopeInfo(0, i);
+    Params.push_back(Parm);
+  }
+  OverloadDecl->setParams(Params);
+  return OverloadDecl;
+}
+
 /// ActOnCallExpr - Handle a call to Fn with the specified array of arguments.
 /// This provides the location of the left/right parens and a list of comma
 /// locations.

   if (UnaryOperator *UnOp = dyn_cast<UnaryOperator>(NakedFn))
     if (UnOp->getOpcode() == UO_AddrOf)
       NakedFn = UnOp->getSubExpr()->IgnoreParens();
-  
-  if (isa<DeclRefExpr>(NakedFn))
+  if (isa<DeclRefExpr>(NakedFn)) {
     NDecl = cast<DeclRefExpr>(NakedFn)->getDecl();
-  else if (isa<MemberExpr>(NakedFn))
+
+    FunctionDecl *FDecl = dyn_cast<FunctionDecl>(NDecl);
+    if (FDecl && FDecl->getBuiltinID()) {
+      // Rewrite the function decl for this builtin by replacing paramaters
+      // with no explicit address space with the address space of the arguments
+      // in ArgExprs.
+      if ((FDecl = rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs))) {
+        NDecl = FDecl;
+        Fn = DeclRefExpr::Create(Context, FDecl->getQualifierLoc(),
+                           SourceLocation(), FDecl, false,
+                           SourceLocation(), FDecl->getType(),
+                           Fn->getValueKind(), FDecl);
+      }
+    }
+  } else if (isa<MemberExpr>(NakedFn))
     NDecl = cast<MemberExpr>(NakedFn)->getMemberDecl();
 
   if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(NDecl)) {

test/CodeGenOpenCL/memcpy.cl

+// RUN: %clang_cc1 %s -ffake-address-space-map -emit-llvm -o - | FileCheck %s
+
+// CHECK-LABEL: @test
+// CHECK-NOT: addrspacecast
+// CHECK: call void @llvm.memcpy.p1i8.p3i8
+kernel void test(global float *g, constant float *c) {
+  __builtin_memcpy(g, c, 32);
+}

test/Sema/builtins.cl

+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic
+// expected-no-diagnostics
+
+kernel void test(global float *out, global float *in, global int* in2) {
+  out[0] = __builtin_nanf("");
+  __builtin_memcpy(out, in, 32);
+  out[0] = __builtin_frexpf(in[0], in2);
+}