<div dir="ltr"><div class="gmail_extra">On Wed, Jun 5, 2013 at 1:09 PM, Tanya Lattner <span dir="ltr"><<a href="mailto:lattner@apple.com" target="_blank">lattner@apple.com</a>></span> wrote:<br><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Regarding the change to SemaType. My one concern is that the type is now "const", which I think is confusing to the user when they see the error message and also I think it emits warnings if someone does const __constant in their source. What I did was change the IsModifiable() function in lib/AST/ExprClassification.cpp to treat anything in constaddr space as ConstQualified in . What do you think about this approach instead?<br>
</blockquote><div><br></div><div>Also worth noting: technically, I think the OpenCL standard requires us to reject this program:</div><div><br></div><div>int f(__constant int*);</div><div>int f(const __constant int* x) {</div>
<div> return *x;</div><div>}</div><div><br></div><div>In practice, I'm not sure anyone would notice.</div><div><br></div><div>-Eli</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Otherwise, the patch looks ok to me.<br>
<br>
-Tanya<br>
<div><div class="h5"><br>
<br>
On May 31, 2013, at 6:35 AM, Joey Gouly <<a href="mailto:joey.gouly@arm.com">joey.gouly@arm.com</a>> wrote:<br>
<br>
> I forgot to add the CodeGen parts of this patch to the review, here it is.<br>
><br>
> <a href="http://llvm-reviews.chandlerc.com/D894" target="_blank">http://llvm-reviews.chandlerc.com/D894</a><br>
><br>
> CHANGE SINCE LAST DIFF<br>
> <a href="http://llvm-reviews.chandlerc.com/D894?vs=2205&id=2209#toc" target="_blank">http://llvm-reviews.chandlerc.com/D894?vs=2205&id=2209#toc</a><br>
><br>
> Files:<br>
> lib/CodeGen/CodeGenModule.cpp<br>
> lib/Frontend/CompilerInvocation.cpp<br>
> lib/Sema/SemaExpr.cpp<br>
> lib/Sema/SemaType.cpp<br>
> test/CodeGenOpenCL/<a href="http://opencl_types.cl" target="_blank">opencl_types.cl</a><br>
> test/CodeGenOpenCL/<a href="http://str_literals.cl" target="_blank">str_literals.cl</a><br>
> test/SemaOpenCL/<a href="http://address-spaces.cl" target="_blank">address-spaces.cl</a><br>
> test/SemaOpenCL/<a href="http://str_literals.cl" target="_blank">str_literals.cl</a><br>
><br>
> Index: lib/CodeGen/CodeGenModule.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CodeGenModule.cpp<br>
> +++ lib/CodeGen/CodeGenModule.cpp<br>
> @@ -2668,11 +2668,16 @@<br>
> llvm::Constant *C =<br>
> llvm::ConstantDataArray::getString(CGM.getLLVMContext(), str, false);<br>
><br>
> + // OpenCL v1.1 s6.5.3: a string literal is in the constant address space.<br>
> + unsigned AddrSpace = 0;<br>
> + if (CGM.getLangOpts().OpenCL)<br>
> + AddrSpace = CGM.getContext().getTargetAddressSpace(LangAS::opencl_constant);<br>
> +<br>
> // Create a global variable for this string<br>
> - llvm::GlobalVariable *GV =<br>
> - new llvm::GlobalVariable(CGM.getModule(), C->getType(), constant,<br>
> - llvm::GlobalValue::PrivateLinkage,<br>
> - C, GlobalName);<br>
> + llvm::GlobalVariable *GV = new llvm::GlobalVariable(<br>
> + CGM.getModule(), C->getType(), constant,<br>
> + llvm::GlobalValue::PrivateLinkage, C, GlobalName, 0,<br>
> + llvm::GlobalVariable::NotThreadLocal, AddrSpace);<br>
> GV->setAlignment(Alignment);<br>
> GV->setUnnamedAddr(true);<br>
> return GV;<br>
> Index: lib/Frontend/CompilerInvocation.cpp<br>
> ===================================================================<br>
> --- lib/Frontend/CompilerInvocation.cpp<br>
> +++ lib/Frontend/CompilerInvocation.cpp<br>
> @@ -1014,6 +1014,7 @@<br>
> Opts.LaxVectorConversions = 0;<br>
> Opts.DefaultFPContract = 1;<br>
> Opts.NativeHalfType = 1;<br>
> + Opts.ConstStrings = 1;<br>
> }<br>
><br>
> if (LangStd == LangStandard::lang_cuda)<br>
> Index: lib/Sema/SemaExpr.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaExpr.cpp<br>
> +++ lib/Sema/SemaExpr.cpp<br>
> @@ -1467,6 +1467,11 @@<br>
> if (getLangOpts().CPlusPlus || getLangOpts().ConstStrings)<br>
> StrTy.addConst();<br>
><br>
> + // OpenCL v1.1 s6.5.3: a string literal is in the constant address space.<br>
> + if (getLangOpts().OpenCL) {<br>
> + StrTy = Context.getAddrSpaceQualType(StrTy, LangAS::opencl_constant);<br>
> + }<br>
> +<br>
> // Get an array type for the string, according to C99 6.4.5. This includes<br>
> // the nul terminator character as well as the string length for pascal<br>
> // strings.<br>
> Index: lib/Sema/SemaType.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaType.cpp<br>
> +++ lib/Sema/SemaType.cpp<br>
> @@ -3829,6 +3829,10 @@<br>
><br>
> unsigned ASIdx = static_cast<unsigned>(addrSpace.getZExtValue());<br>
> Type = S.Context.getAddrSpaceQualType(Type, ASIdx);<br>
> +<br>
> + // Constant address space implies const qualifier<br>
> + if(S.getLangOpts().OpenCL && ASIdx == LangAS::opencl_constant)<br>
> + Type = S.Context.getConstType(Type);<br>
> }<br>
><br>
> /// Does this type have a "direct" ownership qualifier? That is,<br>
> Index: test/CodeGenOpenCL/<a href="http://opencl_types.cl" target="_blank">opencl_types.cl</a><br>
> ===================================================================<br>
> --- test/CodeGenOpenCL/<a href="http://opencl_types.cl" target="_blank">opencl_types.cl</a><br>
> +++ test/CodeGenOpenCL/<a href="http://opencl_types.cl" target="_blank">opencl_types.cl</a><br>
> @@ -1,7 +1,7 @@<br>
> // RUN: %clang_cc1 %s -emit-llvm -o - -O0 | FileCheck %s<br>
><br>
> constant sampler_t glb_smp = 7;<br>
> -// CHECK: global i32 7<br>
> +// CHECK: constant i32 7<br>
><br>
> void fnc1(image1d_t img) {}<br>
> // CHECK: @fnc1(%opencl.image1d_t*<br>
> Index: test/CodeGenOpenCL/<a href="http://str_literals.cl" target="_blank">str_literals.cl</a><br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGenOpenCL/<a href="http://str_literals.cl" target="_blank">str_literals.cl</a><br>
> @@ -0,0 +1,6 @@<br>
> +// RUN: %clang_cc1 %s -emit-llvm -o - -ffake-address-space-map | FileCheck %s<br>
> +<br>
> +__constant char * __constant x = "hello world";<br>
> +<br>
> +// CHECK: addrspace(3) unnamed_addr constant<br>
> +// CHECK: @x = addrspace(3) constant i8 addrspace(3)*<br>
> Index: test/SemaOpenCL/<a href="http://address-spaces.cl" target="_blank">address-spaces.cl</a><br>
> ===================================================================<br>
> --- test/SemaOpenCL/<a href="http://address-spaces.cl" target="_blank">address-spaces.cl</a><br>
> +++ test/SemaOpenCL/<a href="http://address-spaces.cl" target="_blank">address-spaces.cl</a><br>
> @@ -9,5 +9,5 @@<br>
> int *ip;<br>
> ip = gip; // expected-error {{assigning '__global int *' to 'int *' changes address space of pointer}}<br>
> ip = &li; // expected-error {{assigning '__local int *' to 'int *' changes address space of pointer}}<br>
> - ip = &ci; // expected-error {{assigning '__constant int *' to 'int *' changes address space of pointer}}<br>
> + ip = &ci; // expected-error {{assigning 'const __constant int *' to 'int *' changes address space of pointer}}<br>
> }<br>
> Index: test/SemaOpenCL/<a href="http://str_literals.cl" target="_blank">str_literals.cl</a><br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/SemaOpenCL/<a href="http://str_literals.cl" target="_blank">str_literals.cl</a><br>
> @@ -0,0 +1,12 @@<br>
> +// RUN: %clang_cc1 %s -verify<br>
> +// expected-no-diagnostics<br>
> +<br>
> +__constant char * __constant x = "hello world";<br>
> +<br>
> +void foo(__constant char * a) {<br>
> +<br>
> +}<br>
> +<br>
> +void bar() {<br>
> + foo("hello world");<br>
> +}<br>
</div></div>> <D894.3.patch>_______________________________________________<br>
<div class=""><div class="h5">> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>