[OpenCL] Add constant address space to __func__ in AST

[Anastasia]

Summary

String literals must be in constant address space.

[svenvh]

lib/Sema/SemaExpr.cpp

3056 ↗

(On Diff #143886)

Nitpick: local indent style is 2 spaces.

test/SemaOpenCL/predefind-expr.cl

1 ↗

(On Diff #143886)

predefind-expr.cl -> predefined_expr.cl

Also please indent the function body.

[bader]

lib/Sema/SemaExpr.cpp

3059 ↗

(On Diff #143886)

Will it work if we fix this issue inside StringLiteral::Create method?
I just hope it will help us avoid code duplication.

[Anastasia]

• Renamed test;
• Reformatted;
• Added constant in StringLiteral creation.

[bader]

lib/Sema/SemaExpr.cpp

3056 ↗

(On Diff #144154)

Do we still need this?

[Anastasia]

lib/Sema/SemaExpr.cpp

3059 ↗

(On Diff #143886)

I have added the address space to the creation of StringLiteral, but unfortunately it doesn't seems like we can remove similar code in other places because QualType created for StringLiteral is also used elsewhere and has to match (contain right address space). I.e. here is it used further down to create PredefinedExpr.

So not sure there is any value in doing the change in StringLiteral construction really. Unless we could keep it and use getType() from StringLiteral to resent the QualType to be used further down. Let me know what you think about it?

[bader]

lib/AST/Expr.cpp

870 ↗

(On Diff #144154)

As Ty is passed by value, shouldn't we accept only data located in constant address space?

lib/Sema/SemaExpr.cpp

3057 ↗

(On Diff #144154)

String type can only be a constant arrays.
Can we set constant address space inside this getter for OpenCL language?
Or we might want constant array in other address spaces e.g. private?

[Anastasia]

lib/AST/Expr.cpp

870 ↗

(On Diff #144154)

Do you mean to assert? Currently it should be passed with constant AS but I thought the idea is to modify this function so we can accept Default AS too but then replace by constant.

lib/Sema/SemaExpr.cpp

3057 ↗

(On Diff #144154)

Yes, I think we need to be able to declare const arrays in different ASes. I.e. this is valid code in OpenCL:

__private const int* a = ...;

Quick check for getConstantArrayType, it seems to be used in many different place. So I don't think I can just modify it.

[bader]

lib/AST/Expr.cpp

870 ↗

(On Diff #144154)

Yes, but according to your other comment this idea doesn't work.

I have added the address space to the creation of StringLiteral, but unfortunately it doesn't seems like we can remove similar code in other places because QualType created for StringLiteral is also used elsewhere and has to match (contain right address space). I.e. here is it used further down to create PredefinedExpr.

[Anastasia]

lib/AST/Expr.cpp

870 ↗

(On Diff #144154)

Indeed for the current cases it doesn't... there might be some uncaught ones though where it can be useful. But we can add it later as well. So I am thinking to undo this?

[Anastasia]

lib/AST/Expr.cpp

870 ↗

(On Diff #144154)

I quite liked the idea initially, but unfortunately it seems like it's not applicable to the current use cases.

[yaxunl]

Can you add a codegen test for 1.2, 2.0?

Also, I am wondering whether this is valid for OpenCL C++. Do we need special handling for string literal?

lib/AST/Expr.cpp

870 ↗

(On Diff #144154)

Can you extract this as

QualType ASTContext::getStringLiteralType(QualType);

then use it here and below?

test/SemaOpenCL/predefined-expr.cl

1 ↗

(On Diff #144154)

can you also check for cl 2.0?

[Anastasia]

The best way of adding common helper function I could come up with. The problem is that string literals are created quite differently all over: using various char types and const qualifiers. That seems to cover the most common case now.

[Anastasia]

Added accidentally removed code and improved the test.

[Anastasia]

In D46049#1081196, @yaxunl wrote:

Can you add a codegen test for 1.2, 2.0?

Anything specific you would like me to check? I am not modifying anything in CodeGen here though.

Also, I am wondering whether this is valid for OpenCL C++. Do we need special handling for string literal?

Actually OpenCL C++ makes no statement about string literal. But I think we will want it to be in constant AS. I will raise it with Khronos.

[yaxunl]

In D46049#1085388, @Anastasia wrote:

In D46049#1081196, @yaxunl wrote:

Can you add a codegen test for 1.2, 2.0?

Anything specific you would like me to check? I am not modifying anything in CodeGen here though.

You changed the AST of string literal. We need to check whether the CodeGen for string literal still works.

lib/AST/ASTContext.cpp

3632 ↗

(On Diff #144892)

This is not right. The base type of a string literal may not be CharTy, therefore this function needs an input argument which is the original base type and it returns the adjusted base type. (Maybe it should be renamed as adjustStringLiteralBaseType?) Also, you can then use it in StringLiteral::CreateEmpty by passing an empty QualType().

[Anastasia]

• Changed to adjust common function
• Enhanced CodeGen string literal case to also check the predefined expr

[yaxunl]

LGTM. Thanks!

cfe/trunk/include/clang/AST/ASTContext.h

public:
   }
 
   /// Return a normal function type with a typed argument list.
   QualType getFunctionType(QualType ResultTy, ArrayRef<QualType> Args,
                            const FunctionProtoType::ExtProtoInfo &EPI) const {
     return getFunctionTypeInternal(ResultTy, Args, EPI, false);
   }
 
+  QualType adjustStringLiteralBaseType(QualType StrLTy) const;
+
 private:
   /// Return a normal function type with a typed argument list.
   QualType getFunctionTypeInternal(QualType ResultTy, ArrayRef<QualType> Args,
                                    const FunctionProtoType::ExtProtoInfo &EPI,
                                    bool OnlyWantCanonical) const;
 
 public:
   /// Return the unique reference to the type for the specified type

cfe/trunk/lib/AST/ASTContext.cpp

if (!T.isCanonical()) {
     (void)NewIP;
   }
   auto *New = new (*this, TypeAlignment) PipeType(T, Canonical, ReadOnly);
   Types.push_back(New);
   PipeTypes.InsertNode(New, InsertPos);
   return QualType(New, 0);
 }
 
+QualType ASTContext::adjustStringLiteralBaseType(QualType Ty) const {
+  // OpenCL v1.1 s6.5.3: a string literal is in the constant address space.
+  return LangOpts.OpenCL ? getAddrSpaceQualType(Ty, LangAS::opencl_constant)
+                         : Ty;
+}
+
 QualType ASTContext::getReadPipeType(QualType T) const {
   return getPipeType(T, true);
 }
 
 QualType ASTContext::getWritePipeType(QualType T) const {
   return getPipeType(T, false);
 }
 

cfe/trunk/lib/AST/Expr.cpp

StringLiteral *StringLiteral::Create(const ASTContext &C, StringRef Str,
   return SL;
 }
 
 StringLiteral *StringLiteral::CreateEmpty(const ASTContext &C,
                                           unsigned NumStrs) {
   void *Mem =
       C.Allocate(sizeof(StringLiteral) + sizeof(SourceLocation) * (NumStrs - 1),
                  alignof(StringLiteral));
-  StringLiteral *SL = new (Mem) StringLiteral(QualType());
+  StringLiteral *SL =
+      new (Mem) StringLiteral(C.adjustStringLiteralBaseType(QualType()));
   SL->CharByteWidth = 0;
   SL->Length = 0;
   SL->NumConcatenated = NumStrs;
   return SL;
 }
 
 void StringLiteral::outputString(raw_ostream &OS) const {
   switch (getKind()) {

cfe/trunk/lib/Sema/SemaExpr.cpp

if (Literal.isWide()) {
     CharTy = Context.UnsignedCharTy;
   }
 
   QualType CharTyConst = CharTy;
   // A C++ string literal has a const-qualified element type (C++ 2.13.4p1).
   if (getLangOpts().CPlusPlus || getLangOpts().ConstStrings)
     CharTyConst.addConst();
 
+  CharTyConst = Context.adjustStringLiteralBaseType(CharTyConst);
+
   // Get an array type for the string, according to C99 6.4.5.  This includes
   // the nul terminator character as well as the string length for pascal
   // strings.
-  QualType StrTy = Context.getConstantArrayType(CharTyConst,
-                                 llvm::APInt(32, Literal.GetNumStringChars()+1),
-                                 ArrayType::Normal, 0);
+  QualType StrTy = Context.getConstantArrayType(
+      CharTyConst, llvm::APInt(32, Literal.GetNumStringChars() + 1),
 
-  // OpenCL v1.1 s6.5.3: a string literal is in the constant address space.
-  if (getLangOpts().OpenCL) {
-    StrTy = Context.getAddrSpaceQualType(StrTy, LangAS::opencl_constant);
-  }
-
   // Pass &StringTokLocs[0], StringTokLocs.size() to factory!
   StringLiteral *Lit = StringLiteral::Create(Context, Literal.GetString(),
                                              Kind, Literal.Pascal, StrTy,
                                              &StringTokLocs[0],
                                              StringTokLocs.size());
   if (Literal.getUDSuffix().empty())
     return Lit;
 
ExprResult Sema::BuildPredefinedExpr(SourceLocation Loc,
   else {
     // Pre-defined identifiers are of type char[x], where x is the length of
     // the string.
     auto Str = PredefinedExpr::ComputeName(IT, currentDecl);
     unsigned Length = Str.length();
 
     llvm::APInt LengthI(32, Length + 1);
     if (IT == PredefinedExpr::LFunction) {
-      ResTy = Context.WideCharTy.withConst();
+      ResTy =
+          Context.adjustStringLiteralBaseType(Context.WideCharTy.withConst());
       SmallString<32> RawChars;
       ConvertUTF8ToWideString(Context.getTypeSizeInChars(ResTy).getQuantity(),
                               Str, RawChars);
       ResTy = Context.getConstantArrayType(ResTy, LengthI, ArrayType::Normal,
                                            /*IndexTypeQuals*/ 0);
       SL = StringLiteral::Create(Context, RawChars, StringLiteral::Wide,
                                  /*Pascal*/ false, ResTy, Loc);
     } else {
-      ResTy = Context.CharTy.withConst();
+      ResTy = Context.adjustStringLiteralBaseType(Context.CharTy.withConst());
       ResTy = Context.getConstantArrayType(ResTy, LengthI, ArrayType::Normal,
                                            /*IndexTypeQuals*/ 0);
       SL = StringLiteral::Create(Context, Str, StringLiteral::Ascii,
                                  /*Pascal*/ false, ResTy, Loc);
     }
   }
 
   return new (Context) PredefinedExpr(Loc, ResTy, IT, SL);
if (Literal.hasUDSuffix()) {
     }
 
     case LOLR_Raw: {
       // C++11 [lit.ext]p3, p4: If S contains a raw literal operator, the
       // literal is treated as a call of the form
       //   operator "" X ("n")
       unsigned Length = Literal.getUDSuffixOffset();
       QualType StrTy = Context.getConstantArrayType(
-          Context.CharTy.withConst(), llvm::APInt(32, Length + 1),
-          ArrayType::Normal, 0);
+          Context.adjustStringLiteralBaseType(Context.CharTy.withConst()),
+          llvm::APInt(32, Length + 1), ArrayType::Normal, 0);
       Expr *Lit = StringLiteral::Create(
           Context, StringRef(TokSpelling.data(), Length), StringLiteral::Ascii,
           /*Pascal*/false, StrTy, &TokLoc, 1);
       return BuildLiteralOperatorCall(R, OpNameInfo, Lit, TokLoc);
     }
 
     case LOLR_Template: {
       // C++11 [lit.ext]p3, p4: Otherwise (S contains a literal operator
case IncompatiblePointerDiscardsQualifiers: {
     if (SrcType->isArrayType()) SrcType = Context.getArrayDecayedType(SrcType);
 
     Qualifiers lhq = SrcType->getPointeeType().getQualifiers();
     Qualifiers rhq = DstType->getPointeeType().getQualifiers();
     if (lhq.getAddressSpace() != rhq.getAddressSpace()) {
       DiagKind = diag::err_typecheck_incompatible_address_space;
       break;
 
-
     } else if (lhq.getObjCLifetime() != rhq.getObjCLifetime()) {
       DiagKind = diag::err_typecheck_incompatible_ownership;
       break;
     }
 
     llvm_unreachable("unknown error case for discarding qualifiers!");
     // fallthrough
   }

cfe/trunk/test/CodeGenOpenCL/str_literals.cl

 // RUN: %clang_cc1 %s -cl-opt-disable -emit-llvm -o - -ffake-address-space-map | FileCheck %s
 
-__constant char * __constant x = "hello world";
-__constant char * __constant y = "hello world";
+__constant char *__constant x = "hello world";
+__constant char *__constant y = "hello world";
 
-// CHECK: unnamed_addr addrspace(2) constant
+// CHECK: unnamed_addr addrspace(2) constant{{.*}}"hello world\00"
 // CHECK-NOT: addrspace(2) unnamed_addr constant
 // CHECK: @x = {{(dso_local )?}}addrspace(2) constant i8 addrspace(2)*
 // CHECK: @y = {{(dso_local )?}}addrspace(2) constant i8 addrspace(2)*
+// CHECK: unnamed_addr addrspace(2) constant{{.*}}"f\00"
+
+void f() {
+  //CHECK: store i8 addrspace(2)* {{.*}}, i8 addrspace(2)**
+  constant const char *f3 = __func__;
+}

cfe/trunk/test/SemaOpenCL/predefined-expr.cl

+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 %s -verify -cl-std=CL2.0
+
+void f() {
+  char *f1 = __func__;          //expected-error-re{{initializing '{{__generic char|char}} *' with an expression of type 'const __constant char *' changes address space of pointer}}
+  constant char *f2 = __func__; //expected-warning{{initializing '__constant char *' with an expression of type 'const __constant char [2]' discards qualifiers}}
+  constant const char *f3 = __func__;
+}