[OpenCL] Add constant address space to __func__ in AST

[Anastasia]

Summary

String literals must be in constant address space.

[svenvh]

lib/Sema/SemaExpr.cpp

3051

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

3053

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

3051

Do we still need this?

[Anastasia]

lib/Sema/SemaExpr.cpp

3053

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

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

lib/Sema/SemaExpr.cpp

3051

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

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

3051

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

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

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

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

Can you extract this as

QualType ASTContext::getStringLiteralType(QualType);

then use it here and below?

test/SemaOpenCL/predefined-expr.cl

1

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

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!

include/clang/AST/ASTContext.h

public:
   }
 
   /// \brief 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 getStringLiteralBaseType() const;
+
 private:
   /// \brief 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:
   /// \brief Return the unique reference to the type for the specified type

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::getStringLiteralBaseType() const {
+  // OpenCL v1.1 s6.5.3: a string literal is in the constant address space.
+  return LangOpts.OpenCL ? getAddrSpaceQualType(CharTy, LangAS::opencl_constant)
+                         : CharTy;
+}
+
 QualType ASTContext::getReadPipeType(QualType T) const {
   return getPipeType(T, true);
 }
 
 QualType ASTContext::getWritePipeType(QualType T) const {
   return getPipeType(T, false);
 }
 

lib/AST/Expr.cpp

StringLiteral *StringLiteral::Create(const ASTContext &C, StringRef Str,
   // any concatenated string tokens.
   void *Mem =
       C.Allocate(sizeof(StringLiteral) + sizeof(SourceLocation) * (NumStrs - 1),
                  alignof(StringLiteral));
   StringLiteral *SL = new (Mem) StringLiteral(Ty);
 
   // OPTIMIZE: could allocate this appended to the StringLiteral.
   SL->setString(C,Str,Kind,Pascal);
 
   SL->TokLocs[0] = Loc[0];
   SL->NumConcatenated = NumStrs;
 
   if (NumStrs != 1)
     memcpy(&SL->TokLocs[1], Loc+1, sizeof(SourceLocation)*(NumStrs-1));
   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.getLangOpts().OpenCL
+          ? C.getAddrSpaceQualType(QualType(), LangAS::opencl_constant)
+          : QualType());
   SL->CharByteWidth = 0;
   SL->Length = 0;
   SL->NumConcatenated = NumStrs;
   return SL;
 }
 
 void StringLiteral::outputString(raw_ostream &OS) const {
   switch (getKind()) {

lib/Sema/SemaExpr.cpp

Sema::ActOnStringLiteral(ArrayRef<Token> StringToks, Scope *UDLScope) {
   StringLiteralParser Literal(StringToks, PP);
   if (Literal.hadError)
     return ExprError();
 
   SmallVector<SourceLocation, 4> StringTokLocs;
   for (const Token &Tok : StringToks)
     StringTokLocs.push_back(Tok.getLocation());
 
-  QualType CharTy = Context.CharTy;
+  QualType CharTy = Context.getStringLiteralBaseType();
   StringLiteral::StringKind Kind = StringLiteral::Ascii;
   if (Literal.isWide()) {
     CharTy = Context.getWideCharType();
     Kind = StringLiteral::Wide;
   } else if (Literal.isUTF8()) {
     Kind = StringLiteral::UTF8;
   } else if (Literal.isUTF16()) {
     CharTy = Context.Char16Ty;
Sema::ActOnStringLiteral(ArrayRef<Token> StringToks, Scope *UDLScope) {
 
   // 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);
 
-  // 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;
 
   // We're building a user-defined literal.
   IdentifierInfo *UDSuffix = &Context.Idents.get(Literal.getUDSuffix());
   SourceLocation UDSuffixLoc =
     getUDSuffixLoc(*this, StringTokLocs[Literal.getUDSuffixToken()],
                    Literal.getUDSuffixOffset());
if (IT == PredefinedExpr::LFunction) {
       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();
-      if (LangOpts.OpenCL)
-          ResTy = Context.getAddrSpaceQualType(ResTy, LangAS::opencl_constant);
+      ResTy = Context.getStringLiteralBaseType().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);
 }
 
 ExprResult Sema::ActOnPredefinedExpr(SourceLocation Loc, tok::TokenKind Kind) {
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.getStringLiteralBaseType().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: {
 
     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
   }

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{{initializing '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__;
-} 
+  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__;
+}