[PATCH] D50259: [OpenCL] Disallow negative attribute arguments
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang https://reviews.llvm.org/D50259 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46601: [OpenCL] Fix typos in emitted enqueue kernel function names
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Although I think `vaarg` is common too. :) https://reviews.llvm.org/D46601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46667: [OpenCL, OpenMP] Fix crash when OpenMP used in OpenCL file
Anastasia added a comment. OpenCL C is based on C99, so OpenMP isn't enabled by default. But in your tests you use `-fopenmp` to activate it. OpenCL general philosophy is that vectors are written explicitly, but it's not always very easy. In OpenCL C++ we have added an attribute hint for auto vectorization `cl::vec_type_hint`. It's given to a kernel rather than for a loop though. So I think this `simd` pragma is useful. My only worry is that other OpenMP features might not work well in OpenCL and we have no way to reject them at the moment. Repository: rC Clang https://reviews.llvm.org/D46667 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46667: [OpenCL, OpenMP] Fix crash when OpenMP used in OpenCL file
Anastasia added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:11348
// initialiser
-if (var->getTypeSourceInfo()->getType()->isBlockPointerType() &&
-!var->hasInit()) {
+if (var->getType()->isBlockPointerType() && !var->hasInit()) {
Diag(var->getLocation(), diag::err_opencl_invalid_block_declaration)
Does `simd` result in block creation? My original understanding of it was that
it will result in vector operations, not extra threads.
Repository:
rC Clang
https://reviews.llvm.org/D46667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46667: [OpenCL, OpenMP] Fix crash when OpenMP used in OpenCL file
Anastasia added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:11348
// initialiser
-if (var->getTypeSourceInfo()->getType()->isBlockPointerType() &&
-!var->hasInit()) {
+if (var->getType()->isBlockPointerType() && !var->hasInit()) {
Diag(var->getLocation(), diag::err_opencl_invalid_block_declaration)
ABataev wrote:
> Anastasia wrote:
> > Does `simd` result in block creation? My original understanding of it was
> > that it will result in vector operations, not extra threads.
> Currently all of the OpenMP constructs (even simd) create special lambda-like
> construct CapturedStmt that are kind of a block. It is required for unified
> processing of the OpenMP constructs.
I see. It does add some extra code for captures and extra BBs but most of it
seems to be removed with optimizations.
Comment at: test/SemaOpenCL/omp_simd.cl:1
+// RUN: %clang_cc1 -verify -fopenmp -fsyntax-only -x cl %s
+// expected-no-diagnostics
ABataev wrote:
> It would be good to check that at least -ast-print or -ast-dump works
> correctly.
Generated IR seems fine. Might be enough to just convert to IR test?
Repository:
rC Clang
https://reviews.llvm.org/D46667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
Anastasia added inline comments.
Comment at: include/clang/Basic/BuiltinsAMDGPU.def:49
+
+// FIXME: Need to disallow constant address space.
BUILTIN(__builtin_amdgcn_div_scale, "dddbb*", "n")
Do you plan to provide the support for it later? Or if else perhaps we should
elaborate more what's to be done.
Comment at: include/clang/Basic/TargetInfo.h:1009
+ virtual LangAS getOpenCLBuiltinAddressSpace(unsigned AS) const {
+return getLangASFromTargetAS(AS);
Can you add a comment please to explain what the function is for?
Comment at: lib/AST/ASTContext.cpp:9093
unsigned AddrSpace = strtoul(Str, &End, 10);
- if (End != Str && AddrSpace != 0) {
-Type = Context.getAddrSpaceQualType(Type,
-getLangASFromTargetAS(AddrSpace));
+ if (End != Str) {
+// Note AddrSpace == 0 is not the same as an unspecified address space.
Could we check against LangAS::Default instead of removing this completely.
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
Would this be correct for OpenCL? Should we use `isAddressSpaceSupersetOf`
helper instead? Would it also sort the issue with constant AS (at least for
OpenCL)?
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:36
+#if 0
+// XXX: Should this compile?
+void
test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3)))
int *local_ptr, float src) {
`__attribute__((address_space(N)))` is not an OpenCL feature and I think it's
not specified in C either? But I think generally non matching address spaces
don't compile in Clang. So it might be useful to disallow this?
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46501: [OpenCL] Guard all half float usage based on cl_khr_fp16 extension
Anastasia added a comment. In https://reviews.llvm.org/D46501#1089777, @yaxunl wrote: > Only halfn type requires cl_khr_fp16. These functions do not use halfn type, > therefore cl_khr_fp16 is not required. I think the extension is for all half https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xhtml/cl_khr_fp16.html This extension adds support for half scalar and vector types as built-in types that can be used for arithmetic operations, conversions, etc. An application that wants to use half and halfn types will need to include the directive shown above. Repository: rC Clang https://reviews.llvm.org/D46501 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46501: [OpenCL] Guard all half float usage based on cl_khr_fp16 extension
Anastasia added a comment. Could you upload the full diff please, otherwise it's not easy to see all the functions guarded by the macro. Repository: rC Clang https://reviews.llvm.org/D46501 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46667: [OpenCL, OpenMP] Fix crash when OpenMP used in OpenCL file
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Comment at: test/SemaOpenCL/with_openmp.cl:1 +// RUN: %clang_cc1 -verify -fopenmp -ast-dump -x cl %s 2>&1 | FileCheck %s +// expected-no-diagnostics mikerice wrote: > ABataev wrote: > > Still the same question, do we really want to support full OpenMP for > > OpenCL or only simd? > We think it makes sense to have full support. We are using OpenCL for > devices that can run full OpenMP. Is there any known problem with allowing > full support? Sounds good! What target do you use though? Would it help to have an IR test as well? https://reviews.llvm.org/D46667 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37804: [OpenCL] Handle address space conversion while setting type alignment
Anastasia updated this revision to Diff 116548.
Anastasia added a comment.
Addressed comments from Alexey.
https://reviews.llvm.org/D37804
Files:
lib/CodeGen/CGBuilder.h
lib/CodeGen/CGExpr.cpp
test/CodeGenOpenCL/vectorLoadStore.cl
Index: test/CodeGenOpenCL/vectorLoadStore.cl
===
--- test/CodeGenOpenCL/vectorLoadStore.cl
+++ test/CodeGenOpenCL/vectorLoadStore.cl
@@ -1,9 +1,22 @@
-// RUN: %clang_cc1 %s -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 -triple "spir-unknown-unknown" %s -emit-llvm -O0 -o - |
FileCheck %s
-typedef char char3 __attribute((ext_vector_type(3)));;
+typedef char char2 __attribute((ext_vector_type(2)));
+typedef char char3 __attribute((ext_vector_type(3)));
+typedef char char8 __attribute((ext_vector_type(8)));
+typedef float float4 __attribute((ext_vector_type(4)));
// Check for optimized vec3 load/store which treats vec3 as vec4.
void foo(char3 *P, char3 *Q) {
*P = *Q;
// CHECK: %{{.*}} = shufflevector <4 x i8> %{{.*}}, <4 x i8> undef, <3 x
i32>
}
+
+// CHECK: define spir_func void @alignment()
+void alignment() {
+ __private char2 data_generic[100];
+ __private char8 data_private[100];
+
+ // CHECK: %{{.*}} = load <4 x float>, <4 x float> addrspace(4)* %{{.*}},
align 2
+ // CHECK: store <4 x float> %{{.*}}, <4 x float>* %{{.*}}, align 8
+ ((private float4 *)data_private)[1] = ((float4 *)data_generic)[2];
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -925,6 +925,7 @@
// Non-converting casts (but not C's implicit conversion from void*).
case CK_BitCast:
case CK_NoOp:
+case CK_AddressSpaceConversion:
if (auto PtrTy = CE->getSubExpr()->getType()->getAs()) {
if (PtrTy->getPointeeType()->isVoidType())
break;
@@ -953,8 +954,10 @@
CodeGenFunction::CFITCK_UnrelatedCast,
CE->getLocStart());
}
-
-return Builder.CreateBitCast(Addr, ConvertType(E->getType()));
+return CE->getCastKind() != CK_AddressSpaceConversion
+ ? Builder.CreateBitCast(Addr, ConvertType(E->getType()))
+ : Builder.CreateAddrSpaceCast(Addr,
+ ConvertType(E->getType()));
}
break;
Index: lib/CodeGen/CGBuilder.h
===
--- lib/CodeGen/CGBuilder.h
+++ lib/CodeGen/CGBuilder.h
@@ -145,6 +145,13 @@
Addr.getAlignment());
}
+ using CGBuilderBaseTy::CreateAddrSpaceCast;
+ Address CreateAddrSpaceCast(Address Addr, llvm::Type *Ty,
+ const llvm::Twine &Name = "") {
+return Address(CreateAddrSpaceCast(Addr.getPointer(), Ty, Name),
+ Addr.getAlignment());
+ }
+
/// Cast the element type of the given address to a different type,
/// preserving information like the alignment and address space.
Address CreateElementBitCast(Address Addr, llvm::Type *Ty,
Index: test/CodeGenOpenCL/vectorLoadStore.cl
===
--- test/CodeGenOpenCL/vectorLoadStore.cl
+++ test/CodeGenOpenCL/vectorLoadStore.cl
@@ -1,9 +1,22 @@
-// RUN: %clang_cc1 %s -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 -triple "spir-unknown-unknown" %s -emit-llvm -O0 -o - | FileCheck %s
-typedef char char3 __attribute((ext_vector_type(3)));;
+typedef char char2 __attribute((ext_vector_type(2)));
+typedef char char3 __attribute((ext_vector_type(3)));
+typedef char char8 __attribute((ext_vector_type(8)));
+typedef float float4 __attribute((ext_vector_type(4)));
// Check for optimized vec3 load/store which treats vec3 as vec4.
void foo(char3 *P, char3 *Q) {
*P = *Q;
// CHECK: %{{.*}} = shufflevector <4 x i8> %{{.*}}, <4 x i8> undef, <3 x i32>
}
+
+// CHECK: define spir_func void @alignment()
+void alignment() {
+ __private char2 data_generic[100];
+ __private char8 data_private[100];
+
+ // CHECK: %{{.*}} = load <4 x float>, <4 x float> addrspace(4)* %{{.*}}, align 2
+ // CHECK: store <4 x float> %{{.*}}, <4 x float>* %{{.*}}, align 8
+ ((private float4 *)data_private)[1] = ((float4 *)data_generic)[2];
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -925,6 +925,7 @@
// Non-converting casts (but not C's implicit conversion from void*).
case CK_BitCast:
case CK_NoOp:
+case CK_AddressSpaceConversion:
if (auto PtrTy = CE->getSubExpr()->getType()->getAs()) {
if (PtrTy->getPointeeType()->isVoidType())
break;
@@ -953,8 +954,10 @@
CodeGenFunction::CFITCK_UnrelatedCast,
CE->getLocSta
[PATCH] D38134: [OpenCL] Emit enqueued block as kernel
Anastasia added a comment.
I think we should add a test case when the same block is both called and
enqueued.
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:113
+
+llvm::Value *CGOpenCLRuntime::emitOpenCLEnqueuedBlock(CodeGenFunction &CGF,
+ const Expr *E) {
yaxunl wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > I am not particularly in favour of duplicating CodeGen functionality as
> > > > it typically has so many special cases that are hard to catch. Is this
> > > > logic needed in order to pass to block literal information that the
> > > > block is enqueued?
> > > This code is needed to emit separate functions for a block which is
> > > directly called and also enqueued as a kernel. Since the kernel needs to
> > > have proper calling convention and ABI, it cannot be emitted as the same
> > > function as when the block is called directly. Since it is OpenCL
> > > specific code, I found it is cleaner to separate this code as member of
> > > CGOpenCLRuntime instead of fitting it into CGF.EmitBlockLiteral.
> > This part is replacing standard `EmitScalarExpr` call which is doing
> > several things before calling into block generation. That's why I am a bit
> > worried we are covering all the corner cases here.
> >
> > So if we transform all blocks into kernels unconditionally we won't need
> > this special handling then?
> >
> > Do we generate two versions of the blocks now: one for enqueue and one for
> > call?
> If we transform all blocks into kernels, we could simplify the logic.
> Probably will not need this special handling.
>
> However, when the block is called directly, the first argument is a private
> pointer, when it is executed as a kernel, the first argument is a global
> pointer or a struct (for amdgpu target), therefore the emitted functions
> cannot be the same.
Would using generic address space for this first argument not work?
Comment at: test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl:3
+
+// CHECK: %[[S1:struct.__amdgpu_block_arg_t.*]] = type { [3 x i64], [1 x i8] }
+// CHECK: %[[S2:struct.__amdgpu_block_arg_t.*]] = type { [5 x i64], [1 x i8] }
yaxunl wrote:
> Anastasia wrote:
> > This struct is not identical to block literal struct?
> The LLVM type of the first argument of block invoke function is created
> directly with sorting and rearrangement. There is no AST type corresponding
> to it. However, the function codegen requires AST type of this argument. I
> feel it is unnecessary to create the corresponding AST type. For simplicity,
> just create an AST type with the same size and alignment as the LLVM type. In
> the function code, it will be bitcasted to the correct LLVM struct type and
> get the captured variables.
So `void ptr` won't be possible here? Since it is cast to a right struct inside
the block anyway. Once again a block is a special type object with known
semantic to compiler and runtime in contract to kernels that can be written
with any arbitrary type of arguments.
I just don't like the idea of duplicating the block invoke function in case
it's being both called and enqueued. Also the login in blocks code generation
becomes difficult to understand. So I am wondering if we could perhaps create a
separate kernel function (as a wrapper) for enqueue_kernel which would call a
block instead. What do you think about it? I think the kernel prototype would
be fairly generic as it would just have a block call inside and pass the
arguments into it... We won't need to modify block generation then at all.
https://reviews.llvm.org/D38134
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37822: [OpenCL] Clean up and add missing fields for block struct
Anastasia added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:311
+// The header is basically 'struct { int; int; generic void *;
+// custom_fields; }'. Assert that that struct is packed.
+auto GenPtrAlign = CharUnits::fromQuantity(
remove one "that".
Comment at: lib/CodeGen/CGBlocks.cpp:312
+// custom_fields; }'. Assert that that struct is packed.
+auto GenPtrAlign = CharUnits::fromQuantity(
+CGM.getTarget().getPointerAlign(LangAS::opencl_generic) / 8);
I think the alignment might not be computed correctly now if there will be
custom fields that might have a bigger size than a pointer? Also what happens
if we have captures as well?
Comment at: lib/CodeGen/CGBlocks.cpp:850
+ CGM.getDataLayout().getTypeAllocSize(I->getType())),
+ "block.custom");
+ }
do we need to add numeration to each item name?
Comment at: lib/CodeGen/CGBlocks.cpp:1250
// Function
fields.add(blockFn);
If we reorder fields and put this on top we can merge the if statements above
and below this point.
https://reviews.llvm.org/D37822
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38113: OpenCL: Assume functions are convergent
Anastasia added a comment. In https://reviews.llvm.org/D38113#878852, @hfinkel wrote: > In https://reviews.llvm.org/D38113#877874, @Anastasia wrote: > > > The problem of adding this attribute conservatively for all functions is > > that it prevents some optimizations to happen. I agree to commit this as a > > temporary fix to guarantee correctness of generated code. > > > This is one of those unfortunate things we had to do for correctness in CUDA, > and the situation seems the same here. When we're not doing separate > compilation (which I imagine we're also generally not doing for OpenCL > complication), I'm under the impression that the attribute removal is fairly > effective. I agree both communities would benefit so it feels like it might be worth the effort. > > >> But if we ask to add the `convergent` attribute into the spec we can avoid >> doing this in the compiler? > > But even if you do that, would that not be in a future version of OpenCL? If > so, for code complying to current standards, you'd need this behavior. Yes, the fix is needed anyway to provide backwards compatibility. https://reviews.llvm.org/D38113 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38113: OpenCL: Assume functions are convergent
Anastasia added a comment. In https://reviews.llvm.org/D38113#878840, @jlebar wrote: > > Yes, that's why if it would be responsibility of the kernel developer to > > specify this explicitly we could avoid this complications in the compiler. > > But if we add it into the language now we still need to support the > > correctness for the code written with the earlier standards. And also it > > adds the complexity to the programmer to make sure it's specified > > correctly. But I think it is still worth discussing with the spec committee. > > To me this seems like a small complication in the compiler to avoid an > extremely easy bug for users to write. But, not my language. :) Yes, I think I would perhaps argue for inclusion of non-convergent instead since this option seems to make more sense. > > >> The deduction of convergent is indeed tricky. So if there is any function in >> the CFG path which is marked as convergent ( or "non-convergent") this will >> have to be back propagated to the callers unless we force to explicitly >> specify it but it would be too error prone for the kernel writers I guess. > > This probably isn't the right forum to discuss proposals to change the LLVM > IR spec. But if you want to propose something like this, please cc me on the > thread, I probably have opinions. :) Will do! If we have bigger use case it would be easier to get accepted. I will check with the OpenCL community first and see if there is an agreement internally. > > >> Btw, what is the advantage of having "non-convergent" instead and why is the >> deduction of convergent property more complicated with it? > > The advantage of switching LLVM IR to non-convergent would be that front-ends > wouldn't have the bug that arsenm is fixing here. "Unadorned" IR would be > correct. And, in the absence of external or unanalyzable indirect calls, > you'd get the same performance as we get today even if you had no annotations. Yes, I see this sounds more reasonable indeed. Btw, currently LLVM can remove `convergent` in some cases to recover the performance loss? > The complexity I was referring to occurs if you add the non-convergent > attribute and keep the convergent attr. I don't think we want that. I think keeping both would add more confusions and hence result in even more errors/complications. > But I'm not really proposing a change to the convergent attribute in LLVM IR > -- it's probably better to leave it as-is, since we all understand how it > works, it ain't broke. But at the same time since we already know what we needed redesign should be easier. Alternative option would be to add convergent during IR generation as default option and no attribute where `non-convergent` is set. This way at least we give programmer a way to achieve higher performance. But of course it wouldn't be ideal to be inconsistent with the IR. Currently as I can see there is a little use of convergent in the frontend since we set it for all functions anyways. The problem is that it wouldn't be possible to remove it immediately. But we can at least deprecate it for a start. https://reviews.llvm.org/D38113 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: lib/Sema/SemaType.cpp:6808
+static void deduceOpenCLImplicitAddrSpace(TypeProcessingState &State,
+ QualType &T, TypeAttrLocation TAL) {
Great! This looks so clear now!
Comment at: lib/Sema/SemaType.cpp:6810
+ QualType &T, TypeAttrLocation TAL) {
+ if (!State.getSema().getLangOpts().OpenCL ||
+ T.getAddressSpace() != LangAS::Default)
I think this could be checked before calling the function.
Comment at: lib/Sema/SemaType.cpp:6863
+ unsigned ImpAddr;
+ bool IsStatic = D.getDeclSpec().getStorageClassSpec() ==
DeclSpec::SCS_static;
+ // Put OpenCL automatic variable in private address space.
Do we cover `extern` too?
Comment at: lib/Sema/SemaType.cpp:6872
+ ImpAddr = LangAS::opencl_private;
+else if (IsStatic)
+ ImpAddr = LangAS::opencl_global;
I think we can't have this case for CL1.2 see s6.8. But I think it could happen
for `extern` though.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37804: [OpenCL] Handle address space conversion while setting type alignment
Anastasia added a comment. Committed in r314304 In https://reviews.llvm.org/D37804#882252, @alekseyshl wrote: > vectorLoadStore.cl is failing on our bots: > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/8187, > please check it out I will commit a fix in a bit. Repository: rL LLVM https://reviews.llvm.org/D37804 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37804: [OpenCL] Handle address space conversion while setting type alignment
Anastasia added a comment. Committed in r314317. Repository: rL LLVM https://reviews.llvm.org/D37804 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37822: [OpenCL] Clean up and add missing fields for block struct
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM! Thanks!
Comment at: test/CodeGenOpenCL/blocks.cl:30
+ // COMMON: %[[block_captured:.*]] = getelementptr inbounds <{ i32, i32, i8
addrspace(4)*, i32 }>, <{ i32, i32, i8 addrspace(4)*, i32 }>* %[[block]], i32
0, i32 3
+ // COMMON: %[[r0:.*]] = load i32, i32* %i
+ // COMMON: store i32 %[[r0]], i32* %[[block_captured]],
It might be better to give those r0-r7 some names for readability if possible!
https://reviews.llvm.org/D37822
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: lib/Sema/SemaType.cpp:6872
+ ImpAddr = LangAS::opencl_private;
+else if (IsStatic)
+ ImpAddr = LangAS::opencl_global;
yaxunl wrote:
> Anastasia wrote:
> > I think we can't have this case for CL1.2 see s6.8. But I think it could
> > happen for `extern` though.
> Right. I will remove setting implicit addr space for static var for CL1.2.
>
> For extern var, for CL2.0 I will set implicit addr space to global.
>
> However, for CL1.2 since only constant addr space is supported for file-scope
> var, I can only set the implicit addr space of an extern var to be constant.
> However I feel this may cause more confusion than convenience, therefore I
> will not set implicit addr space for extern var for CL1.2. If user does not
> use constant addr space with extern var explicitly, they will see diagnostics
> that extern var must have constant addr space. This is also the current
> behavior before my change.
Makes sense!
Comment at: test/SemaOpenCL/storageclass.cl:63
+ static float l_implicit_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
+ static constant float l_constant_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
+ static global float l_global_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
Does it make sense to put different address spaces here since this code is
rejected earlier anyway?
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38134: [OpenCL] Emit enqueued block as kernel
Anastasia added inline comments.
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:144
+ if (auto *I = dyn_cast(V)) {
+// If the block literal is emitted as an instruction, it is an alloca
+// and the block invoke function is stored to GEP of this alloca.
Why do we need to replace original block calls with the kernels? I think in
case of calling a block we could use the original block function and only for
enqueue use the kernel that would call the block function inside. The pointer
to the kernel wrapper could be passed as an additional parameter to
`enqueue_kernel` calls. We won't need to iterate through all IR then.
Comment at: lib/CodeGen/TargetInfo.cpp:8927
+llvm::Function *
+TargetCodeGenInfo::createEnqueuedBlockKernel(CodeGenFunction &CGF,
+ llvm::Function *Invoke,
Could you add some comments please?
Comment at: lib/CodeGen/TargetInfo.cpp:8949
+ Builder.restoreIP(IP);
+ return F;
+}
Wondering if we should add the kernel metadata (w/o args) since it was used for
long time to indicate the kernel.
Comment at: lib/CodeGen/TargetInfo.h:35
class Decl;
+class ASTContext;
Do we need this?
Comment at: test/CodeGenOpenCL/cl20-device-side-enqueue.cl:9
-// N.B. The check here only exists to set BL_GLOBAL
-// COMMON: @block_G = addrspace(1) constant void (i8 addrspace(3)*)
addrspace(4)* addrspacecast (void (i8 addrspace(3)*) addrspace(1)* bitcast ({
i32, i32, i8 addrspace(4)* } addrspace(1)*
[[BL_GLOBAL:@__block_literal_global(\.[0-9]+)?]] to void (i8 addrspace(3)*)
addrspace(1)*) to void (i8 addrspace(3)*) addrspace(4)*)
+// COMMON: %struct.__opencl_block_literal_generic = type { i32, i32, i8
addrspace(4)* }
+
Can we check generated kernel function too?
https://reviews.llvm.org/D38134
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added a comment. In https://reviews.llvm.org/D35082#890162, @rjmccall wrote: > Okay. I think I see your point about why it would be better to have a > canonical __private address space that is different from the implicit address > space 0. I assume this means that there should basically never be pointers, > references, or l-values in address space 0 in OpenCL. If you mean 0 is `private` address space then no. There can be private AS pointers. But if you mean 0 is no address space then this doesn't exist in OpenCL. I think the right design in the first place would have been to keep address spaces in AST just as they are in the source code and add explicit address spaces to all types in IR instead. In this case absence of any address spaces in AST would signal implicit AS to be used. This would make some Sema checks a bit more complicated though, but the design would become a lot cleaner. Unfortunately I think it would not be worth doing this big change now. > You will lose a significant amount of representational efficiency by doing > this, but it's probably not overwhelming. > > I know you aren't implementing OpenCL C++ yet, so most of the cases where > temporaries are introduced aren't meaningful to you, but you will at least > need to consider compound literals. I suspect the right rule is that > file-scope literals should be inferred as being in __global or __constant > memory, depending on whether they're const, and local-scope literals should > be inferred as __private. > > I'll try to review your patch tomorrow. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38113: OpenCL: Assume functions are convergent
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D38113 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38134: [OpenCL] Emit enqueued block as kernel
Anastasia added inline comments.
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:144
+ if (auto *I = dyn_cast(V)) {
+// If the block literal is emitted as an instruction, it is an alloca
+// and the block invoke function is stored to GEP of this alloca.
yaxunl wrote:
> Anastasia wrote:
> > Why do we need to replace original block calls with the kernels? I think in
> > case of calling a block we could use the original block function and only
> > for enqueue use the kernel that would call the block function inside. The
> > pointer to the kernel wrapper could be passed as an additional parameter to
> > `enqueue_kernel` calls. We won't need to iterate through all IR then.
> `CGF.EmitScalarExpr(Block)` returns the block literal structure which
> contains the size/align/invoke_function/captures. The block invoke function
> is stored to the struct by a `StoreInst`. To create the wrapper kernel, we
> need to get the block invoke function, therefore we have to iterate through
> IR.
>
> Since we need to find the store instruction any way, it is simpler to just
> replace the stored function with the kernel and pass the block literal
> struct, instead of passing the kernel separately.
So we cann't get the invoke function from the block literal structure passed
into the kernel wrapper directly knowing its offset? Iterating through IR adds
extra time and also I am not sure how reliable this is wrt different corner
cases of IR.
Comment at: lib/CodeGen/TargetInfo.cpp:8949
+ Builder.restoreIP(IP);
+ return F;
+}
yaxunl wrote:
> Anastasia wrote:
> > Wondering if we should add the kernel metadata (w/o args) since it was used
> > for long time to indicate the kernel.
> Currently (before this change), clang already does not generate kernel
> metadata if there is no vec_type_hint, work_group_size_hint,
> reqd_work_group_size. Remember last time we made the change to use function
> metadata to represent these attributes. Whether a function is a kernel can be
> determined by its calling convention.
Ok, let's leave it for now. We can always add it in on request.
Comment at: test/CodeGenOpenCL/cl20-device-side-enqueue.cl:297
+// COMMON: define internal spir_kernel void [[INVG5]](i8 addrspace(4)*{{.*}},
i8 addrspace(3)*{{.*}})
+// COMMON: define internal spir_kernel void [[INVG6]](i8 addrspace(4)*{{.*}},
i8 addrspace(3)*{{.*}}, i8 addrspace(3)*{{.*}}, i8 addrspace(3)*{{.*}})
+// COMMON: define internal spir_kernel void [[INVG7]](i8 addrspace(4)*{{.*}},
i8 addrspace(3)*{{.*}})
Perhaps we could check the body of this one too since it has a different
prototype.
https://reviews.llvm.org/D38134
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added a comment. > (*) I know that you aren't considering OpenCL C++ yet, but often these > representation/model questions are easier to answer when thinking about C++ > instead of C because type differences are so much more directly important in > C++. In OpenCL C++, I assume it will be important to distinguish between > and <__private int> as template arguments. That tells us straight up > that __private needs to be represented differently from a lack of qualifier. > Note that the result, where certain type representations (e.g. an unqualified > int*) become basically meaningless and should never be constructed by Sema, > is already precedented in Clang: ObjC ARC does the same thing to unqualified > ObjC pointer types, like id*. It's pretty complicated in OpenCL as `int` and `private int` won't be different, but `int*` and `private int*` would be different only in OpenCL2.0. The rules are pretty convoluted and I have summarized them in the table earlier: https://reviews.llvm.org/D35082#inline-327303. The issue is that there is not anything like this in other languages and therefore we have this issue trying to fit this concept with something similar but not exactly the same. I thought you and Sam decided to use implicit AS flag to make printing messages consistent with the original source code. I am fine with this approach, however, I would prefer to just keep no AS qualifier in the default AS mode instead of deducing it during parcing in `processTypeAttrs`, and only to add the AS to the IR at the end of the CodeGen phase. I think it would be a lot easier to understand. However, as Sam has pointed out a lot of code in Sema has been written assuming the deduction of AS and is using AS qualifiers explicitly and therefore it seems like it would be a bigger change to go for. At the same type we have refactored the deduction of default AS now in the parcing phase and it looks a lot better than I thought it would be. So I don't mind if we continue this way. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33681: [OpenCL] Allow function declaration with empty argument list.
Anastasia accepted this revision. Anastasia added a comment. I would prefer to merge the new test with test/SemaOpenCL/func.cl, but otherwise LGTM! Thanks! https://reviews.llvm.org/D33681 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33989: [OpenCL] Allow targets to select address space per type
Anastasia added inline comments.
Comment at: cfe/trunk/lib/Basic/TargetInfo.cpp:351
+LangAS::ID TargetInfo::getOpenCLTypeAddrSpace(const Type *T) const {
+ auto BT = dyn_cast(T);
chapuni wrote:
> Excuse me for old commit, I think it might be layering violation.
> Could we avoid using AST/Type here?
One of the problems is that even if we could write another layer of enums to
map OpenCL specific AST types into the TargetInfo representation it doesn't
really help creating a layered structure between AST and Basic libs. They seem
to have a large logical dependence despite not including the library headers
physically. So modification in AST would require modifications in Basic anyway
resulting in overhead of changing 2 places instead of 1. So I am wondering if
there is any better approach here e.g. revisiting the library dependencies or
what classes they should contain?
Repository:
rL LLVM
https://reviews.llvm.org/D33989
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38134: [OpenCL] Emit enqueued block as kernel
Anastasia added a comment.
I think it would be good to add a block test to CodeGenOpenCL where we would
just call the block without any enqueue and check that the invoke function is
generated but the kernel wrapper isn't.
Comment at: lib/CodeGen/CGBuiltin.cpp:2846
+ PtrToSizeArray};
+ std::vector ArgTys = {QueueTy,
+ IntTy,
Formatting seems inconsistent from above.
Comment at: lib/CodeGen/CodeGenFunction.h:2921
private:
- /// Helpers for blocks
- llvm::Value *EmitBlockLiteral(const CGBlockInfo &Info);
+ /// Helpers for blocks. Returns invoke function by \p InvokeF if it is not
+ /// nullptr.
It will be nullptr in case block is not enqueued? May be it's worth explaining
it in the comment.
Comment at: lib/CodeGen/TargetInfo.h:290
}
+ /// Create an OpenCL kernel for an enqueued block.
+ virtual llvm::Function *
Can we also explain the wrapper kernel here?
https://reviews.llvm.org/D38134
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
Anastasia added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
Why do we need this change?
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38868: [OpenCL] Restrict swizzle length check to OpenCL mode
Anastasia added inline comments.
Comment at: test/SemaOpenCL/vector_swizzle_length.cl:7
void foo() {
-float8 f2 = (float8)(0, 0, 0, 0, 0, 0, 0, 0);
+float8 f2 = (float8){0, 0, 0, 0, 0, 0, 0, 0};
Even though this works in Clang, ideally OpenCL vector literal is with
parenthesis (see s6.1.6).
Comment at: test/SemaOpenCL/vector_swizzle_length.cl:13
+#else
+// expected-no-diagnostics
+(void)f2.s01234;
I am not sure it's good idea to test C mode here since this is intended for
OpenCL only functionality. I think it might be better to separate C
functionality and keep under regular Sema tests. We could keep the same test
name to be able to identify similar functionality.
https://reviews.llvm.org/D38868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
Anastasia added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
arichardson wrote:
> Anastasia wrote:
> > Why do we need this change?
> `__attribute__((address_space(n)))` is a target address space and not a
> language address space like `LangAS::opencl_generic`. Isn't
> `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for this use
> case?
Yes, I think there are some adjustment we do in this method to get the original
source value to be printed corerctly. Does this mean we have no tests that
caught this issue?
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38857: [OpenCL] Improve printing and semantic check related to implicit addr space
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM! Thanks!
Can we close https://bugs.llvm.org/show_bug.cgi?id=33418 after this commit?
Comment at: test/SemaOpenCL/null_literal.cl:38
-#ifdef CL20
-// Accept explicitly pointer to generic address space in OpenCL v2.0.
-global int* ptr5 = (generic void*)0;
-#endif
-
-global int* ptr6 = (local void*)0; // expected-error{{initializing '__global
int *' with an expression of type '__local void *' changes address space of
pointer}}
+ global int *g7 = (global void*)(generic void *)0;
+ constant int *c7 = (constant void*)(generic void *)0;
//expected-error{{casting '__generic void *' to type '__constant void *'
changes address space of pointer}}
Does this extra cast test anything we already miss to test?
https://reviews.llvm.org/D38857
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38134: [OpenCL] Emit enqueued block as kernel
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Great work! Thanks! https://reviews.llvm.org/D38134 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
Anastasia added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
arichardson wrote:
> Anastasia wrote:
> > arichardson wrote:
> > > Anastasia wrote:
> > > > Why do we need this change?
> > > `__attribute__((address_space(n)))` is a target address space and not a
> > > language address space like `LangAS::opencl_generic`. Isn't
> > > `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for this
> > > use case?
> > Yes, I think there are some adjustment we do in this method to get the
> > original source value to be printed corerctly. Does this mean we have no
> > tests that caught this issue?
> Seems like it, all tests pass both with and without this patch.
Strange considering that we have this attribute printed in some error messages
of some Sema tests. If I compile this code without your patch:
```
typedef int __attribute__((address_space(1))) int_1;
typedef int __attribute__((address_space(2))) int_2;
void f0(int_1 &);
void f0(const int_1 &);
void test_f0() {
int i;
static int_2 i2;
f0(i);
f0(i2);
}
```
I get the address spaces printed correctly inside the type:
note: candidate function not viable: 1st argument ('int_2' (aka
'__attribute__((address_space(2))) int')) is in address space 2, but parameter
must be in address space 1
Perhaps @yaxunl could comment further on whether this change is needed.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38868: [OpenCL] Restrict swizzle length check to OpenCL mode
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: test/SemaOpenCL/vector_swizzle_length.cl:7
void foo() {
-float8 f2 = (float8)(0, 0, 0, 0, 0, 0, 0, 0);
+float8 f2 = (float8){0, 0, 0, 0, 0, 0, 0, 0};
bruno wrote:
> Anastasia wrote:
> > Even though this works in Clang, ideally OpenCL vector literal is with
> > parenthesis (see s6.1.6).
> Ok. I changed this to avoid warnings in C mode. Gonna change it back,
Can you undo this change please before committing. Thanks!
https://reviews.llvm.org/D38868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39129: [OpenCL] Fix generation of constant address space sampler in function scope
Anastasia created this revision.
After the change of constant address space function scope variable in 6e34f0e
("[OpenCL] Emit function-scope variable in constant address space as static
variable", 2017-05-15) a bug has been introduced that triggered unreachable
during generation of samplers.
This is because sampler in global scope has not to be generated. The
initialization function has to be used instead of a variable everywhere the
sampler variable is being referenced.
This patch fixes the bug and adds missing test case.
https://reviews.llvm.org/D39129
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGenOpenCL/sampler.cl
Index: test/CodeGenOpenCL/sampler.cl
===
--- test/CodeGenOpenCL/sampler.cl
+++ test/CodeGenOpenCL/sampler.cl
@@ -33,6 +33,10 @@
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)*
@__translate_sampler_initializer(i32 19)
// CHECK: store %opencl.sampler_t addrspace(2)* [[SAMP]], %opencl.sampler_t
addrspace(2)** [[smp_ptr]]
+ // Initialising constant AS sampler will be handled as global (we won't
generate local alloca for it)
+ // CHECK-NOT: alloca %opencl.sampler_t addrspace(2)*
+ constant sampler_t smp_const_as = 11;
+
// Case 1b
fnc4smp(smp);
// CHECK-NOT: call %opencl.sampler_t addrspace(2)*
@__translate_sampler_initializer(i32 19)
@@ -58,4 +62,8 @@
fnc4smp(5);
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)*
@__translate_sampler_initializer(i32 5)
// CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)*
[[SAMP]])
+
+ fnc4smp(smp_const_as);
+ // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)*
@__translate_sampler_initializer(i32 11)
+ // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)*
[[SAMP]])
}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -158,6 +158,13 @@
// Don't emit it now, allow it to be emitted lazily on its first use.
return;
+ // Samplers in constant address space are handled as a special case
+ // unlike other variables they are not generated as globals
+ // and their initialiser will be used everywhere it is being referenced.
+ if (D.getType().getAddressSpace() == LangAS::opencl_constant &&
+ D.getType().getTypePtr()->isSamplerT())
+ return;
+
// Some function-scope variable does not have static storage but still
// needs to be emitted like a static variable, e.g. a function-scope
// variable in constant address space in OpenCL.
Index: test/CodeGenOpenCL/sampler.cl
===
--- test/CodeGenOpenCL/sampler.cl
+++ test/CodeGenOpenCL/sampler.cl
@@ -33,6 +33,10 @@
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19)
// CHECK: store %opencl.sampler_t addrspace(2)* [[SAMP]], %opencl.sampler_t addrspace(2)** [[smp_ptr]]
+ // Initialising constant AS sampler will be handled as global (we won't generate local alloca for it)
+ // CHECK-NOT: alloca %opencl.sampler_t addrspace(2)*
+ constant sampler_t smp_const_as = 11;
+
// Case 1b
fnc4smp(smp);
// CHECK-NOT: call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 19)
@@ -58,4 +62,8 @@
fnc4smp(5);
// CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 5)
// CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* [[SAMP]])
+
+ fnc4smp(smp_const_as);
+ // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 11)
+ // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* [[SAMP]])
}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -158,6 +158,13 @@
// Don't emit it now, allow it to be emitted lazily on its first use.
return;
+ // Samplers in constant address space are handled as a special case
+ // unlike other variables they are not generated as globals
+ // and their initialiser will be used everywhere it is being referenced.
+ if (D.getType().getAddressSpace() == LangAS::opencl_constant &&
+ D.getType().getTypePtr()->isSamplerT())
+ return;
+
// Some function-scope variable does not have static storage but still
// needs to be emitted like a static variable, e.g. a function-scope
// variable in constant address space in OpenCL.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51212: [OpenCL][Docs] Release notes for OpenCL in Clang
Anastasia created this revision. Anastasia added reviewers: hans, bader, yaxunl. https://reviews.llvm.org/D51212 Files: docs/ReleaseNotes.rst Index: docs/ReleaseNotes.rst === --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -222,10 +222,31 @@ ... -OpenCL C Language Changes in Clang --- +OpenCL C/C++ Language Changes in Clang +-- -... +Miscellaneous changes in OpenCL C: + +- Added ``cles_khr_int64`` extension. + +- Added bug fixes and simplifications to Clang blocks in OpenCL mode. + +- Added compiler flag ``-cl-uniform-work-group-size`` to allow extra compile time optimisation. + +- Propagate ``denorms-are-zero`` attribute to IR if ``-cl-denorms-are-zero`` is passed to the compiler. + +- Separated ``read_only`` and ``write_only`` pipe IR types. + +- Fixed address space for the ``__func__`` predefined macro. + +- Improved diagnostics of kernel argument types. + + +Started OpenCL C++ support: + +- Added ``-std/-cl-std=c++``. + +- Added support for keywords. OpenMP Support in Clang -- Index: docs/ReleaseNotes.rst === --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -222,10 +222,31 @@ ... -OpenCL C Language Changes in Clang --- +OpenCL C/C++ Language Changes in Clang +-- -... +Miscellaneous changes in OpenCL C: + +- Added ``cles_khr_int64`` extension. + +- Added bug fixes and simplifications to Clang blocks in OpenCL mode. + +- Added compiler flag ``-cl-uniform-work-group-size`` to allow extra compile time optimisation. + +- Propagate ``denorms-are-zero`` attribute to IR if ``-cl-denorms-are-zero`` is passed to the compiler. + +- Separated ``read_only`` and ``write_only`` pipe IR types. + +- Fixed address space for the ``__func__`` predefined macro. + +- Improved diagnostics of kernel argument types. + + +Started OpenCL C++ support: + +- Added ``-std/-cl-std=c++``. + +- Added support for keywords. OpenMP Support in Clang -- ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43783: [OpenCL] Remove block invoke function from emitted block literal struct
Anastasia added a comment.
In https://reviews.llvm.org/D43783#1212485, @yaxunl wrote:
> In https://reviews.llvm.org/D43783#1204353, @svenvh wrote:
>
> > Sorry for digging up an old commit...
> >
> > Apparently this broke block arguments, e.g. the following test case:
> >
> > int foo(int (^ bl)(void)) {
> > return bl();
> > }
> >
> > int get21() {
> > return foo(^{return 21;});
> > }
> >
> > int get42() {
> > return foo(^{return 42;});
> > }
> >
> >
> > In particular, the VarDecl that `getBlockExpr()` sees doesn't have an
> > initializer when the called block comes from an argument (causing clang to
> > crash).
>
>
> Sorry for the delay. I think block should not be allowed as function argument
> since this generally leads indirect function calls therefore requires support
> of function pointer. It will rely on optimizations to get rid of indirect
> function calls.
The idea was to allow blocks as function parameters because they are statically
known at each function call. This can be resolved later at IR level instead of
frontend. I am also not sure there can be other corner cases where it wouldn't
work in Clang since we can't leverage analysis passes here. I feel this might
be a risky thing to do in Clang. Currently it doesn't work for the examples
provided and therefore breaking the compliance with the spec.
Repository:
rC Clang
https://reviews.llvm.org/D43783
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51212: [OpenCL][Docs] Release notes for OpenCL in Clang
Anastasia added a comment. In https://reviews.llvm.org/D51212#1214173, @hans wrote: > Anastasia: will you commit this to the branch, or would like me to do it? I will commit this! Thanks! https://reviews.llvm.org/D51212 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51296: [Sema] Traverse vector types for ocl extensions support
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Could you please change the commit title tag: [Sema] -> [OpenCL] Comment at: lib/Sema/Sema.cpp:42 #include "llvm/ADT/SmallSet.h" + using namespace clang; It would be better to undo this formatting change. Clang code base seems to be inconsistent on that anyway. Repository: rC Clang https://reviews.llvm.org/D51296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51302: [OpenCL] Relax diagnostics on OpenCL access qualifiers
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM! Thanks!
Comment at: test/SemaOpenCL/access-qualifier.cl:107
+
+kernel void image_wo_twice(write_only write_only image1d_t i){} //
expected-warning {{duplicate 'write_only' declaration specifier}}
+kernel void image_wo_twice_typedef(write_only img1d_wo i){} //
expected-warning {{duplicate 'write_only' declaration specifier}}
Could we change one `write_only` to `__write_only` to increase test coverage.
Repository:
rC Clang
https://reviews.llvm.org/D51302
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51341: [HEADER] Overloadable function candidates for half/double types
Anastasia added inline comments.
Comment at: lib/Sema/SemaOverload.cpp:6063
+ // OpenCL: A candidate function that uses extentions that are not enabled or
+ // supported is not viable.
+ if (getLangOpts().OpenCL) {
I would imagine if extension isn't supported the candidate function with data
type defined by extension shouldn't be available at all during compilation?
Also is there any good way to generalize this for all types and extensions
including vendor ones that are added with the pragmas?
https://clang.llvm.org/docs/UsersManual.html#opencl-extensions
Comment at: test/SemaOpenCL/half-double-overload.cl:7
+
+int __attribute__((overloadable)) goo(float in1, float in2);
+half __attribute__((overloadable)) goo(double in1, double in2);
I think it will be clearer if candidates with non-half parameters moved out of
extension enabled block.
Comment at: test/SemaOpenCL/half-double-overload.cl:18
+float __attribute__((overloadable)) foo_err(half in1, half in2);
+// expected-note@-1 {{candidate disabled due to OpenCL extension}}
+float __attribute__((overloadable)) foo_err(half in1, int in2);
Wondering if better message could be:
candidate unavailable as it requires OpenCL extension to be disabled
Could it also print the extension name?
https://reviews.llvm.org/D51341
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51341: [HEADER] Overloadable function candidates for half/double types
Anastasia added a comment. I think this is related to: https://reviews.llvm.org/D46501 https://reviews.llvm.org/D51341 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51402: [OpenCL] Adding cl_intel_planar_yuv extension
Anastasia added a comment. Will this extension require any Clang changes that are not possible to add using this approach for vendor extensions: https://clang.llvm.org/docs/UsersManual.html#opencl-extensions If not I would suggest to add this to the header file instead. Repository: rC Clang https://reviews.llvm.org/D51402 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51411: [OpenCL] Improve diagnostic of argument in address space conversion builtins
Anastasia added a comment. In https://reviews.llvm.org/D51411#1217477, @yaxunl wrote: > Is this a feature requested by users? > > I can understand that this may be useful to pinpoint some conversions which > are potentially harmful to performance. However such conversions can often be > eliminated by optimization, which makes this warning less useful. > > On the other hand, too many warnings is a nuance to users, therefore I am > wondering whether this warning should be off by default. > > Do you have any chance to have any feedback from users about how useful or > intrusive this warning is. Hi Sam, no feedback from the user. If you think this is not that useful we won't commit or make is off by default as you suggested. Let us know what you think makes more sense. https://reviews.llvm.org/D51411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51484: [OpenCL] Add support of cl_intel_device_side_avc_motion_estimation extension
Anastasia added inline comments. Comment at: include/clang/Basic/OpenCLExtensionTypes.def:1 +//===-- OpenCLExtensionTypes.def - Metadata about BuiltinTypes --*- C++ -*-===// +// I am confused about the purpose of this file. Is it supposed to contain intel extension specific types or generally OpenCL types? Comment at: include/clang/Basic/OpenCLExtensionTypes.def:27 + +INTEL_SGAVC_TYPE(mce_payload_t, McePayload) +INTEL_SGAVC_TYPE(ime_payload_t, ImePayload) From the specification of this extension I can't quite see if these types have to be in Clang instead of the header. Can you please elaborate on any example where it wouldn't be possible for this type to be declared in the header using the technique explained in: https://clang.llvm.org/docs/UsersManual.html#opencl-extensions Comment at: lib/Headers/opencl-c.h:16197 +#ifdef cl_intel_device_side_avc_motion_estimation +#pragma OPENCL EXTENSION cl_intel_device_side_avc_motion_estimation : enable + Should we be using: #pragma OPENCL EXTENSION my_ext : begin then the user can get correct diagnostics that the functions can only be used once the extension is enabled. Repository: rC Clang https://reviews.llvm.org/D51484 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51544: [OpenCL] Split opencl-c.h header
Anastasia added a comment. It seems generally good to partition this big header but I am trying to understand what problem is it trying to solve now? The unsupported declarations are guarded out by `#if defined(ext_name)` and therefore won't be parsed and put into PCH is extensions are not supported by some target. So I guess it can save the space when installing the headers because we don't need to copy all of them into the installation? Although this is not implemented currently. Repository: rC Clang https://reviews.llvm.org/D51544 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51722: [OpenCL] Allow blocks to capture arrays in OpenCL
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM! There is a small comment that can be addressed before the final commit.
Thanks!
Comment at: lib/Sema/SemaExpr.cpp:14627
- // Blocks are not allowed to capture arrays.
- if (CaptureType->isArrayType()) {
+ // Blocks are not allowed to capture arrays, excepting OpenCL.
+ if (!S.getLangOpts().OpenCL && CaptureType->isArrayType()) {
It would be good to add spec reference here.
Repository:
rC Clang
https://reviews.llvm.org/D51722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51402: [OpenCL] Adding cl_intel_planar_yuv extension
Anastasia added inline comments. Comment at: lib/Headers/opencl-c.h:26 +#if __OPENCL_C_VERSION__ >= CL_VERSION_1_2 +#ifndef cl_intel_planar_yuv +#define cl_intel_planar_yuv @yaxunl, do you think we need to add some kind of architecture guard for such things? Like it should only be added if the architecture supports the extension? But I guess `-cl-ext=+cl_intel_planar_yuv` trick might not work here because it's not a Clang known extension? So may be the right solution here is to introduce a target specific header? For now it can be explicitly included but we could think of a target hook to preload a target specific header... https://reviews.llvm.org/D51402 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51484: [OpenCL] Add support of cl_intel_device_side_avc_motion_estimation extension
Anastasia added inline comments. Comment at: include/clang/Basic/OpenCLExtensionTypes.def:27 + +INTEL_SGAVC_TYPE(mce_payload_t, McePayload) +INTEL_SGAVC_TYPE(ime_payload_t, ImePayload) AlexeySachkov wrote: > Anastasia wrote: > > From the specification of this extension I can't quite see if these types > > have to be in Clang instead of the header. Can you please elaborate on any > > example where it wouldn't be possible for this type to be declared in the > > header using the technique explained in: > > https://clang.llvm.org/docs/UsersManual.html#opencl-extensions > We cannot define these types in header because their layout is not defined in > specification, i.e. all of these types are opaque This is not the reason to add functionality to Clang. You can easily sort such things with target specific headers or even general headers (see `ndrange_t` for example). Spec doesn't have to describe everything. The question is whether there is something about those types that can't be handled using standard include mechanisms. Usually it's prohibited behaviors that can't be represented with such mechanisms. Like if some operations have to be disallowed or allowed (since in OpenCL C you can't define user defined operators) with the types. I think the trend is to avoid adding complexity into Clang, unless there is no other way to implement some feature correctly. Repository: rC Clang https://reviews.llvm.org/D51484 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51544: [OpenCL] Split opencl-c.h header
Anastasia added a comment. In https://reviews.llvm.org/D51544#1224780, @asavonic wrote: > In https://reviews.llvm.org/D51544#1224730, @Anastasia wrote: > > > It seems generally good to partition this big header but I am trying to > > understand what problem is it trying to solve now? > > > Main motivation is to reduce memory footprint by factoring out everything > that is 'common' between PCHs compiled for different targets (CL1.2/2.0 and > spir/spir64). > > > So if we want to add these 2 devices, we now need 4*2 different PCHs, > > since every combination of -cl-std and -triple must be compiled twice > > with different OpenCL extensions defines. > > > > Size of each PCH is 2.5M, so we need ~20M of memory to store our > > PCHs. If we want to add more devices or support another OpenCL C > > version, the size will double. > > This also allows to do 'partial' pre-compilation, where 'common' part is > pre-compiled into a single target independent PCH, and plain header is used > for all target-specific things. > In this case, you no longer need to maintain different target-specific PCHs > and this greatly simplifies the whole process. > > > opencl-c-platform.h (5K LOC) must still be pre-compiled for each > > supported combination, but since it is a lot smaller (~0.5M vs > > original 2.5M), it is not that bad. Or we can sacrifice some > > performance and leave this header without pre-compilation: large > > portion of opencl-c-platform.h contains vendor extensions, so it will > > be removed by preprocessor anyway. Currently the main header still contains everything, so the size of the PCH won't change. So I was wondering what would be the way to avoid this... other than adding explicit `#include`. Repository: rC Clang https://reviews.llvm.org/D51544 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51544: [OpenCL] Split opencl-c.h header
Anastasia added a comment. In https://reviews.llvm.org/D51544#1227336, @asavonic wrote: > In https://reviews.llvm.org/D51544#1227313, @Anastasia wrote: > > > Currently the main header still contains everything, so the size of the PCH > > won't change. > > > The idea is that we don't pre-compile the whole opencl-c.h, we split > it into several headers (3 of them are target independent) and > pre-compile them instead. > > With this approach, we can reuse target-independent PCHs (common, > fp16, fp64) and only duplicate target-specific PCHs if needed > (opencl-c-platform.h). > > The idea is basically: > > 1. Compile target-independent headers into modules: > - opencl-c-common.h -> opencl-c-common.pcm > - opencl-c-fp16.h -> opencl-c-fp16.pcm > - opencl-c-fp64.h -> opencl-c-fp64.pcm > 2. Implicitly include opencl-c.h (plain header), which has the following > content: > > > > #include "opencl-c-common.h" > #if cl_khr_fp16 > #include "opencl-c-fp16.h" > #endif > #if cl_khr_fp64 > #include "opencl-c-fp16.h" > #endif > #include "opencl-c-platform.h" > > > When compiler reaches an #include statement in opencl-c.h, it loads a > corresponding PCH. Headers that were not pre-compiled are included as > usual. Ok, I see. It seems reasonable. I am not sure if `opencl-c-platform` is the right name, since it seems some of the bits are just version/extension dependent there and not the platform? But perhaps if you add some description with guidelines on what should be put in each header, it should be fine. Repository: rC Clang https://reviews.llvm.org/D51544 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51544: [OpenCL] Split opencl-c.h header
Anastasia added a comment. > With this setup, we can compile opencl-c-common.h, opencl-c-fp16.h and > opencl-c-fp64.h into PCHs with one set of extensions/OpenCL version, > and use them for any other set of extensions/OpenCL version. Clang > will detect this and throw out an error, which can be safely disabled > by -fno-validate-pch option. However, keeping this as a permanent solution is unsafe. Because this way can result in unexpected errors to be silent out and allow erroneous configurations to be accepted successfully without any notification. So I am wondering if there is any plan to put a proper solution in place at some point? Repository: rC Clang https://reviews.llvm.org/D51544 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51341: [HEADER] Overloadable function candidates for half/double types
Anastasia added inline comments.
Comment at: lib/Sema/SemaOverload.cpp:6063
+ // OpenCL: A candidate function that uses extentions that are not enabled or
+ // supported is not viable.
+ if (getLangOpts().OpenCL) {
sidorovd wrote:
> Anastasia wrote:
> > I would imagine if extension isn't supported the candidate function with
> > data type defined by extension shouldn't be available at all during
> > compilation?
> >
> > Also is there any good way to generalize this for all types and extensions
> > including vendor ones that are added with the pragmas?
> > https://clang.llvm.org/docs/UsersManual.html#opencl-extensions
> > I would imagine if extension isn't supported the candidate function with
> > data type defined by extension shouldn't be available at all during
> > compilation?
>
> Yes, you are right.
>
> > Also is there any good way to generalize this for all types and extensions
> > including vendor ones that are added with the pragmas?
> https://clang.llvm.org/docs/UsersManual.html#opencl-extensions
>
> There might be a way but I can't properly answer this question right now. By
> default types and extensions aren't associated to each other.
We have a map of types and associated extensions with them in
`OpenCLTypeExtMap` in Sema.h... not sure what would be the cost of such generic
check though.
Comment at: test/SemaOpenCL/half-double-overload.cl:18
+float __attribute__((overloadable)) foo_err(half in1, half in2);
+// expected-note@-1 {{candidate disabled due to OpenCL extension}}
+float __attribute__((overloadable)) foo_err(half in1, int in2);
sidorovd wrote:
> Anastasia wrote:
> > Wondering if better message could be:
> > candidate unavailable as it requires OpenCL extension to be disabled
> >
> > Could it also print the extension name?
> Sounds good. And I'd prefer to split it into another patch, because it would
> affect unrelated to the change tests and also require changes in Sema to
> allow extension name printing.
Ok, makes sense for this to be a separate change!
https://reviews.llvm.org/D51341
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51544: [OpenCL] Split opencl-c.h header
Anastasia added a comment. In https://reviews.llvm.org/D51544#1230264, @asavonic wrote: > In https://reviews.llvm.org/D51544#1229105, @Anastasia wrote: > > > > With this setup, we can compile opencl-c-common.h, opencl-c-fp16.h and > > > opencl-c-fp64.h into PCHs with one set of extensions/OpenCL version, > > > and use them for any other set of extensions/OpenCL version. Clang > > > will detect this and throw out an error, which can be safely disabled > > > by -fno-validate-pch option. > > > > However, keeping this as a permanent solution is unsafe. Because this way > > can result in unexpected errors to be silent out and allow erroneous > > configurations to be accepted successfully without any notification. > > > Agree. LIT test in `test/Headers/opencl-c-header-split.cl` is supposed > to verify that common/fp16/fp64 headers do not use preprocessor macros > and it should catch most of the issues, but this is definitely not the > most robust solution. > > > So I am wondering if there is any plan to put a proper solution in place at > > some point? > > This solution can be improved if we implement `convert` and `shuffle` > as clang builtins with custom typechecking: these two builtins (with > all their overloadings) take ~90% of `opencl-c-common.h` and ~50% of > fp16/fp64 headers. > > However, this can be a non-trivial change: it is difficult to do a > proper mangling for clang builtins and rounding modes must be handled > as well. > > I'll take a few days to prototype this. If it turns out to be good in > terms of performance/footprint, we can drop this patch. Btw, I was wondering if a target agnostic PCH is a viable solution to move forward too. Something that we discussed/prototyped during the HackerLab in EuroLLVM? Repository: rC Clang https://reviews.llvm.org/D51544 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43783: [OpenCL] Remove block invoke function from emitted block literal struct
Anastasia added a comment. Ping! Do you still plan to do this? :) Repository: rC Clang https://reviews.llvm.org/D43783 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51411: [OpenCL] Improve diagnostic of argument in address space conversion builtins
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D51411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49723: [OpenCL] Check for invalid kernel arguments in array types
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Repository: rC Clang https://reviews.llvm.org/D49723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49725: [OpenCL] Forbid size dependent types used as kernel arguments
Anastasia added inline comments. Comment at: lib/Sema/SemaDecl.cpp:8186 - const RecordDecl *PD = PT->castAs()->getDecl(); - VisitStack.push_back(PD); + // At this point we already handled everything except of a RecordType or + // an ArrayType[RecordType]. I am a bit confused about this comment, `do you mean a PointerType to a RecordType or an ArrayType of a RecordType`? Comment at: lib/Sema/SemaDecl.cpp:8186 - const RecordDecl *PD = PT->castAs()->getDecl(); - VisitStack.push_back(PD); + // At this point we already handled everything except of a RecordType or + // an ArrayType[RecordType]. Anastasia wrote: > I am a bit confused about this comment, `do you mean a PointerType to a > RecordType or an ArrayType of a RecordType`? Also is there any test case covering this change? Comment at: lib/Sema/SemaDecl.cpp:8189 + const RecordType *RecTy = + PT->getPointeeOrArrayElementType()->getAs(); + const RecordDecl *OrigRecDecl = RecTy->getDecl(); yaxunl wrote: > Can we have a test for this change? e.g. an array of structs I am wondering if `PT->getPointeeOrArrayElementType()` is `nullptr`? Do we need to add an extra check? Repository: rC Clang https://reviews.llvm.org/D49725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49723: [OpenCL] Check for invalid kernel arguments in array types
Anastasia added a comment. Btw, has this restriction been removed from CL 2.0? Repository: rC Clang https://reviews.llvm.org/D49723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49725: [OpenCL] Forbid size dependent types used as kernel arguments
Anastasia added inline comments. Comment at: lib/Sema/SemaDecl.cpp:8267 - S.Diag(PD->getLocation(), diag::note_within_field_of_type) -<< PD->getDeclName(); + S.Diag(OrigRecDecl->getLocation(), diag::note_within_field_of_type) + << OrigRecDecl->getDeclName(); Should this bit go into https://reviews.llvm.org/D49725? Repository: rC Clang https://reviews.llvm.org/D49725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49725: [OpenCL] Forbid size dependent types used as kernel arguments
Anastasia accepted this revision. Anastasia added a comment. LGTM! Thanks! Repository: rC Clang https://reviews.llvm.org/D49725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49723: [OpenCL] Check for invalid kernel arguments in array types
Anastasia added a comment. In https://reviews.llvm.org/D49723#1174837, @asavonic wrote: > In https://reviews.llvm.org/D49723#1173352, @Anastasia wrote: > > > Btw, has this restriction been removed from CL 2.0? > > > No, it applies for CL2.0 as well. It seems however the restriction on pointer to pointer was removed (see s6.9.a last item) in CL2.0. Comment at: lib/Sema/SemaDecl.cpp:8187 + // walk around RecordDecl::fields(). + assert((PT->isArrayType() || PT->isRecordType()) && "Unexpected type."); + const Type *FieldRecTy = Field->getType()->getPointeeOrArrayElementType(); Do we need to assert PT here too? It doesn't seem to be modified in this loop... Repository: rC Clang https://reviews.llvm.org/D49723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47618: __c11_atomic_load's _Atomic can be const
Anastasia added inline comments.
Comment at: test/SemaOpenCL/atomic-ops.cl:61
__opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
- __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group); //
expected-error {{address argument to atomic operation must be a pointer to
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic
_Atomic(int) *') invalid)}}
+ __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
Could we add a line with constant AS pointer:
__opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group);
Just to make sure we still give an error for this case.
Repository:
rC Clang
https://reviews.llvm.org/D47618
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49723: [OpenCL] Check for invalid kernel arguments in array types
Anastasia accepted this revision. Anastasia added a comment. LGTM! Thanks! Repository: rC Clang https://reviews.llvm.org/D49723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM from OpenCL side! Thanks!
Comment at: test/SemaOpenCL/numbered-address-space.cl:9
+void
test_numeric_as_to_generic_explicit_cast(__attribute__((address_space(3))) int
*as3_ptr, float src) {
+ generic int* generic_ptr = (generic int*) as3_ptr; // Should maybe be valid?
+}
Ideally this is not governed by any specification. Generic compiler support for
such `addrspacecast` is not possible but vendor can implement custom support. I
think we can leave it as is until we have better idea how this should be
supported.
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47618: __c11_atomic_load's _Atomic can be const
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM from OpenCL side! Thanks! Repository: rC Clang https://reviews.llvm.org/D47618 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42844: [OpenCL] Add test for atomic pointers.
Anastasia added a comment. Sorry for delay... Does this test anything OpenCL specific? As far as I remember we don't have any changes to `PointerType` with an atomic pointee type. Repository: rC Clang https://reviews.llvm.org/D42844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42844: [OpenCL] Add test for atomic pointers.
Anastasia added a comment. In https://reviews.llvm.org/D42844#1004663, @Fznamznon wrote: > Yes, we don't have anything OpenCL specific for atomic pointers. > But we have OpenCL specific in pointers and atomics separately and we have > address spaces. > Why not to test all at once? We don't typically do integration testing in Clang to keep the runtime of tests within reasonable bounds. Even though this test is quite small I would say if it doesn't increase our code coverage (test a program path which is not tested yet) I wouldn't be so keen to add it. As for the pointer type implementation I don't think this test is testing any new behavior to what we already cover by C pointer tests. I don't remember any special rule in OpenCL wrt operations on either atomics or local pointers. Repository: rC Clang https://reviews.llvm.org/D42844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43240: [OpenCL] Fix __enqueue_block for block with captures
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks for looking at this. Just one thing, I was wondering whether it would be cleaner way if we extend test/CodeGenOpenCL/cl20-device-side-enqueue.cl instead of adding a new one here? Because this is the test that is meant to exercise all DSE codegen bits. Perhaps we can modify one block in that test to have the same format as here (i.e. using captures), since now we test the same block there most of the time. However, we don't test any of kernel wrapper `*_block_invoke_kernel` there. Not sure why... https://reviews.llvm.org/D43240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43570: [OpenCL] Add '-cl-uniform-work-group-size' compile option
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Repository: rC Clang https://reviews.llvm.org/D43570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31745: [OpenCL] Added diagnostic for implicit declaration of function in OpenCL
Anastasia added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:12449
// function declaration is going to be treated as an error.
- if (Diags.getDiagnosticLevel(diag_id, Loc) >= DiagnosticsEngine::Error) {
+ if (!getLangOpts().OpenCL &&
+ Diags.getDiagnosticLevel(diag_id, Loc) >= DiagnosticsEngine::Error) {
echuraev wrote:
> Anastasia wrote:
> > This way prevents the typo corrections completely for OpenCL which is not
> > very desirable. I was just wondering could we prevent adding the invalid
> > builtin function identifiers instead to the correction candidate list.
> >
> > Like when `work_group_reserve_read_pipe` was first parsed it shouldn't have
> > been added to the list of valid function identifiers to appear in the
> > corrections of 'work_group_reserve_write_pipe'. I am guessing the
> > identifier might be added when builtins are initialized...
> Yes, sorry, I didn't think about it. I investigated the question how can I
> remove invalid functions from correction candidate list. But I have a problem
> and I hope that you will be able to help me.
>
> I found that the correction is added in function `void
> TypoCorrectionConsumer::addName` (in file //SemaLookup.cpp//) which called
> from loop in function `std::unique_ptr
> Sema::makeTypoCorrectionConsumer`. The loop you can see below:
>
> ```
> // For unqualified lookup, look through all of the names that we have
> // seen in this translation unit.
> // FIXME: Re-add the ability to skip very unlikely potential corrections.
> for (const auto &I : Context.Idents)
> Consumer->FoundName(I.getKey());
> ```
>
> But the map `Context.Idents` already contains names of implicit functions.
> So, I found that names of functions were added to this dictionary during
> parsing AST. After calling `ConsumeToken()` function in `void
> Parser::ParseDeclarationSpecifiers` (in file //ParseDecl.cpp//) implicit
> functions were added to the dictionary. But in this function I'm not able to
> check is the OpenCL function implicit declared or not.
>
> As a result I tried to remove names of implicit functions before calling
> `CorrectTypo`. But unfortunately, we don't have an API for removing items
> from `Context.Idents`. So, I don't know the good way for fixing these
> diagnostic messages. Could you help me please? Do you have any suggestions?
>
> Thank you in advance!
>
Thanks for looking at this!
It seems like we are adding the identifiers to the map too early. So we could:
(a) postpose adding items into `Context.Idents` to `Sema`.
(b) extend `IdentifierTable` interface to support removing items (although it
might not align well with the general concept).
Although (b) feels more natural way conceptually, it might require bigger
changes than (a).
Both need discussion in `cfe-dev` to continue, so it should be addressed in
isolation. If you undo the last change, I am happy to approve the review.
https://reviews.llvm.org/D31745
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33353: [OpenCL] An error shall occur if any scalar operand has greater rank than the type of the vector element
Anastasia added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8307
"variable length arrays are not supported in OpenCL">;
+def err_scalar_type_rank_greater_than_vector_type : Error<
+"scalar operand type has greater rank than the type of the vector "
Since it's OpenCL specific rule, could we rename:
err_scalar_type_rank_greater_than_vector_type ->
err_opencl_scalar_type_rank_greater_than_vector_type
Comment at: lib/Sema/SemaExpr.cpp:8344
// the vector element type and splat.
+ unsigned DeclID = 0;
if (!RHSVecType) {
Could we initialize this with `diag::err_typecheck_vector_not_convertable` and
then just use default return with empty QualType at the end of the function
instead of adding an extra one in lines 8369-8374?
https://reviews.llvm.org/D33353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33483: [OpenCL] reserve_id_t cannot be used as argument to kernel function
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Btw the same applies to the program scope declaration (s6.9.p), but could be done as a separate change. https://reviews.llvm.org/D33483 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33489: [OpenCL] Added regression test on invalid vector initialization.
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D33489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33353: [OpenCL] An error shall occur if any scalar operand has greater rank than the type of the vector element
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D33353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31745: [OpenCL] Added diagnostic for implicit declaration of function in OpenCL
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D31745 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33592: [OpenCL] Test on half immediate support.
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D33592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33597: [OpenCL] Fix pipe size in TypeInfo.
Anastasia created this revision.
Herald added a subscriber: yaxunl.
Pipes are now the size of pointers rather than the size of the type that they
contain.
Patch by Simon Perretta!
https://reviews.llvm.org/D33597
Files:
lib/AST/ASTContext.cpp
test/Index/pipe-size.cl
Index: test/Index/pipe-size.cl
===
--- /dev/null
+++ test/Index/pipe-size.cl
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
spir-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
spir64-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR64
+__kernel void testPipe( pipe int test )
+{
+int s = sizeof(test);
+// X86: store %opencl.pipe_t* %test, %opencl.pipe_t** %test.addr, align 8
+// X86: store i32 8, i32* %s, align 4
+// SPIR: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t
addrspace(1)** %test.addr, align 4
+// SPIR: store i32 4, i32* %s, align 4
+// SPIR64: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t
addrspace(1)** %test.addr, align 8
+// SPIR64: store i32 8, i32* %s, align 4
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1939,9 +1939,8 @@
break;
case Type::Pipe: {
-TypeInfo Info = getTypeInfo(cast(T)->getElementType());
-Width = Info.Width;
-Align = Info.Align;
+Width = Target->getPointerWidth(0);
+Align = Target->getPointerAlign(0);
}
}
Index: test/Index/pipe-size.cl
===
--- /dev/null
+++ test/Index/pipe-size.cl
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple spir-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple spir64-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR64
+__kernel void testPipe( pipe int test )
+{
+int s = sizeof(test);
+// X86: store %opencl.pipe_t* %test, %opencl.pipe_t** %test.addr, align 8
+// X86: store i32 8, i32* %s, align 4
+// SPIR: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t addrspace(1)** %test.addr, align 4
+// SPIR: store i32 4, i32* %s, align 4
+// SPIR64: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t addrspace(1)** %test.addr, align 8
+// SPIR64: store i32 8, i32* %s, align 4
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1939,9 +1939,8 @@
break;
case Type::Pipe: {
-TypeInfo Info = getTypeInfo(cast(T)->getElementType());
-Width = Info.Width;
-Align = Info.Align;
+Width = Target->getPointerWidth(0);
+Align = Target->getPointerAlign(0);
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33598: [libclang] [OpenCL] Expose CIndex functions for typedef and address space
Anastasia created this revision.
Expose the following functions:
clang_getTypedefName
clang_getAddressSpace
Patch by Simon Perretta!
https://reviews.llvm.org/D33598
Files:
bindings/python/clang/cindex.py
bindings/python/tests/cindex/test_type.py
include/clang-c/Index.h
tools/libclang/CXType.cpp
tools/libclang/libclang.exports
Index: tools/libclang/libclang.exports
===
--- tools/libclang/libclang.exports
+++ tools/libclang/libclang.exports
@@ -147,6 +147,7 @@
clang_findReferencesInFileWithBlock
clang_formatDiagnostic
clang_free
+clang_getAddressSpace
clang_getAllSkippedRanges
clang_getArgType
clang_getArrayElementType
@@ -259,6 +260,7 @@
clang_getTypeKindSpelling
clang_getTypeSpelling
clang_getTypedefDeclUnderlyingType
+clang_getTypedefName
clang_hashCursor
clang_indexLoc_getCXSourceLocation
clang_indexLoc_getFileLocation
Index: tools/libclang/CXType.cpp
===
--- tools/libclang/CXType.cpp
+++ tools/libclang/CXType.cpp
@@ -21,6 +21,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/Type.h"
+#include "clang/Basic/AddressSpaces.h"
#include "clang/Frontend/ASTUnit.h"
using namespace clang;
@@ -394,6 +395,27 @@
return T.isLocalRestrictQualified();
}
+unsigned clang_getAddressSpace(CXType CT) {
+ QualType T = GetQualType(CT);
+
+ // For non language-specific address space, use separate helper function.
+ if (T.getAddressSpace() >= LangAS::Count) {
+ return T.getQualifiers().getAddressSpaceAttributePrintValue();
+ }
+ return T.getAddressSpace();
+}
+
+CXString clang_getTypedefName(CXType CT) {
+ QualType T = GetQualType(CT);
+ const TypedefType *TT = T->getAs();
+ if (TT) {
+TypedefNameDecl *TD = TT->getDecl();
+if (TD)
+ return cxstring::createDup(TD->getNameAsString().c_str());
+ }
+ return cxstring::createEmpty();
+}
+
CXType clang_getPointeeType(CXType CT) {
QualType T = GetQualType(CT);
const Type *TP = T.getTypePtrOrNull();
Index: include/clang-c/Index.h
===
--- include/clang-c/Index.h
+++ include/clang-c/Index.h
@@ -3404,6 +3404,16 @@
CINDEX_LINKAGE unsigned clang_isRestrictQualifiedType(CXType T);
/**
+ * \brief Returns the address space of the given type.
+ */
+CINDEX_LINKAGE unsigned clang_getAddressSpace(CXType T);
+
+/**
+ * \brief Returns the typedef name of the given type.
+ */
+CINDEX_LINKAGE CXString clang_getTypedefName(CXType CT);
+
+/**
* \brief For pointer types, returns the type of the pointee.
*/
CINDEX_LINKAGE CXType clang_getPointeeType(CXType T);
Index: bindings/python/tests/cindex/test_type.py
===
--- bindings/python/tests/cindex/test_type.py
+++ bindings/python/tests/cindex/test_type.py
@@ -37,37 +37,44 @@
assert not fields[0].type.is_const_qualified()
assert fields[0].type.kind == TypeKind.INT
assert fields[0].type.get_canonical().kind == TypeKind.INT
+assert fields[0].type.get_typedef_name() == ''
assert fields[1].spelling == 'b'
assert not fields[1].type.is_const_qualified()
assert fields[1].type.kind == TypeKind.TYPEDEF
assert fields[1].type.get_canonical().kind == TypeKind.INT
assert fields[1].type.get_declaration().spelling == 'I'
+assert fields[1].type.get_typedef_name() == 'I'
assert fields[2].spelling == 'c'
assert not fields[2].type.is_const_qualified()
assert fields[2].type.kind == TypeKind.LONG
assert fields[2].type.get_canonical().kind == TypeKind.LONG
+assert fields[2].type.get_typedef_name() == ''
assert fields[3].spelling == 'd'
assert not fields[3].type.is_const_qualified()
assert fields[3].type.kind == TypeKind.ULONG
assert fields[3].type.get_canonical().kind == TypeKind.ULONG
+assert fields[3].type.get_typedef_name() == ''
assert fields[4].spelling == 'e'
assert not fields[4].type.is_const_qualified()
assert fields[4].type.kind == TypeKind.LONG
assert fields[4].type.get_canonical().kind == TypeKind.LONG
+assert fields[4].type.get_typedef_name() == ''
assert fields[5].spelling == 'f'
assert fields[5].type.is_const_qualified()
assert fields[5].type.kind == TypeKind.INT
assert fields[5].type.get_canonical().kind == TypeKind.INT
+assert fields[5].type.get_typedef_name() == ''
assert fields[6].spelling == 'g'
assert not fields[6].type.is_const_qualified()
assert fields[6].type.kind == TypeKind.POINTER
assert fields[6].type.get_pointee().kind == TypeKind.INT
+assert fields[6].type.get_typedef_name() == ''
assert fields[7].spelling == 'h'
assert not fields[7].type.is_const_qualified()
@@ -75,6 +82,7 @@
assert fields[7].type.get_pointee().kind == TypeKind.POINTER
assert fields[7].type.get_pointee().get_point
[PATCH] D33706: [AMDGPU] Fix address space for global and temporary variables in C++
Anastasia added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:1107
assert(T.getAddressSpace() == LangAS::Default);
if (getASTAllocaAddressSpace() != LangAS::Default) {
auto *Addr = getTargetHooks().performAddrSpaceCast(
Is this code still needed here, considering that CreateTempAlloca is doing this
already?
Comment at: lib/CodeGen/CGDecl.cpp:1109
- // Alloca always returns a pointer in alloca address space, which may
- // be different from the type defined by the language. For example,
Why removing the comment?
Comment at: lib/CodeGen/CodeGenFunction.h:1930
+ /// temporary variable in default address space. If \p DoCast is true,
+ /// alloca will be casted to the address space expected by the language,
+ /// otherwise it stays in the alloca address space.
I am still confused, what is the case the alloca AS is different from the
language AS?
https://reviews.llvm.org/D33706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33597: [OpenCL] Fix pipe size in TypeInfo.
Anastasia updated this revision to Diff 101031.
Anastasia edited reviewers, added: yaxunl; removed: echuraev.
Anastasia removed a subscriber: yaxunl.
Anastasia added a comment.
Use global AS pointer for pipe size.
@Sam, I am moving you to the reviewer to finish this change. Do you think it
makes sense to add RUN line with some AMD GPU in triple to the test?
https://reviews.llvm.org/D33597
Files:
lib/AST/ASTContext.cpp
test/Index/pipe-size.cl
Index: test/Index/pipe-size.cl
===
--- /dev/null
+++ test/Index/pipe-size.cl
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
spir-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
spir64-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR64
+__kernel void testPipe( pipe int test )
+{
+int s = sizeof(test);
+// X86: store %opencl.pipe_t* %test, %opencl.pipe_t** %test.addr, align 8
+// X86: store i32 8, i32* %s, align 4
+// SPIR: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t
addrspace(1)** %test.addr, align 4
+// SPIR: store i32 4, i32* %s, align 4
+// SPIR64: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t
addrspace(1)** %test.addr, align 8
+// SPIR64: store i32 8, i32* %s, align 4
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1939,9 +1939,8 @@
break;
case Type::Pipe: {
-TypeInfo Info = getTypeInfo(cast(T)->getElementType());
-Width = Info.Width;
-Align = Info.Align;
+Width =
Target->getPointerWidth(getTargetAddressSpace(LangAS::opencl_global));
+Align =
Target->getPointerAlign(getTargetAddressSpace(LangAS::opencl_global));
}
}
Index: test/Index/pipe-size.cl
===
--- /dev/null
+++ test/Index/pipe-size.cl
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple spir-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple spir64-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR64
+__kernel void testPipe( pipe int test )
+{
+int s = sizeof(test);
+// X86: store %opencl.pipe_t* %test, %opencl.pipe_t** %test.addr, align 8
+// X86: store i32 8, i32* %s, align 4
+// SPIR: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t addrspace(1)** %test.addr, align 4
+// SPIR: store i32 4, i32* %s, align 4
+// SPIR64: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t addrspace(1)** %test.addr, align 8
+// SPIR64: store i32 8, i32* %s, align 4
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1939,9 +1939,8 @@
break;
case Type::Pipe: {
-TypeInfo Info = getTypeInfo(cast(T)->getElementType());
-Width = Info.Width;
-Align = Info.Align;
+Width = Target->getPointerWidth(getTargetAddressSpace(LangAS::opencl_global));
+Align = Target->getPointerAlign(getTargetAddressSpace(LangAS::opencl_global));
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33597: [OpenCL] Fix pipe size in TypeInfo.
Anastasia updated this revision to Diff 101236.
Anastasia added a comment.
Added RUN line for AMD
https://reviews.llvm.org/D33597
Files:
lib/AST/ASTContext.cpp
test/Index/pipe-size.cl
Index: test/Index/pipe-size.cl
===
--- /dev/null
+++ test/Index/pipe-size.cl
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
spir-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
spir64-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR64
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple
amdgcn-amd-amdhsa-amdgizcl %s -o - | FileCheck %s --check-prefix=AMD
+__kernel void testPipe( pipe int test )
+{
+int s = sizeof(test);
+// X86: store %opencl.pipe_t* %test, %opencl.pipe_t** %test.addr, align 8
+// X86: store i32 8, i32* %s, align 4
+// SPIR: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t
addrspace(1)** %test.addr, align 4
+// SPIR: store i32 4, i32* %s, align 4
+// SPIR64: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t
addrspace(1)** %test.addr, align 8
+// SPIR64: store i32 8, i32* %s, align 4
+// AMD: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t
addrspace(1)* addrspace(5)* %test.addr, align 4
+// AMD: store i32 8, i32 addrspace(5)* %s, align 4
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1939,9 +1939,8 @@
break;
case Type::Pipe: {
-TypeInfo Info = getTypeInfo(cast(T)->getElementType());
-Width = Info.Width;
-Align = Info.Align;
+Width =
Target->getPointerWidth(getTargetAddressSpace(LangAS::opencl_global));
+Align =
Target->getPointerAlign(getTargetAddressSpace(LangAS::opencl_global));
}
}
Index: test/Index/pipe-size.cl
===
--- /dev/null
+++ test/Index/pipe-size.cl
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple spir-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple spir64-unknown-unknown %s -o - | FileCheck %s --check-prefix=SPIR64
+// RUN: %clang_cc1 -x cl -O0 -cl-std=CL2.0 -emit-llvm -triple amdgcn-amd-amdhsa-amdgizcl %s -o - | FileCheck %s --check-prefix=AMD
+__kernel void testPipe( pipe int test )
+{
+int s = sizeof(test);
+// X86: store %opencl.pipe_t* %test, %opencl.pipe_t** %test.addr, align 8
+// X86: store i32 8, i32* %s, align 4
+// SPIR: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t addrspace(1)** %test.addr, align 4
+// SPIR: store i32 4, i32* %s, align 4
+// SPIR64: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t addrspace(1)** %test.addr, align 8
+// SPIR64: store i32 8, i32* %s, align 4
+// AMD: store %opencl.pipe_t addrspace(1)* %test, %opencl.pipe_t addrspace(1)* addrspace(5)* %test.addr, align 4
+// AMD: store i32 8, i32 addrspace(5)* %s, align 4
+}
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1939,9 +1939,8 @@
break;
case Type::Pipe: {
-TypeInfo Info = getTypeInfo(cast(T)->getElementType());
-Width = Info.Width;
-Align = Info.Align;
+Width = Target->getPointerWidth(getTargetAddressSpace(LangAS::opencl_global));
+Align = Target->getPointerAlign(getTargetAddressSpace(LangAS::opencl_global));
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33821: [OpenCL] Harden function pointer diagnostics.
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D33821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33598: [libclang] [OpenCL] Expose CIndex functions for typedef and address space
Anastasia added a comment. Sam, do you think you have some time to look at this change? Thanks! https://reviews.llvm.org/D33598 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33681: Allow function declaration with empty argument list.
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D33681 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34235: [OpenCL] Fix OpenCL and SPIR version metadata generation.
Anastasia added inline comments.
Comment at: test/CodeGenOpenCL/spir_version.cl:13
kernel void foo() {}
+kernel void bar() {}
Would the original code produce duplicate version metadata here or is it just
for overloaded functions? Would it make sense to add `CHECK-NOT` to make sure
they are not generated twice?
https://reviews.llvm.org/D34235
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33989: [OpenCL] Allow targets to select address space per type
Anastasia added inline comments. Comment at: include/clang/Basic/TargetInfo.h:1041 +default: + return LangAS::Default; +} bader wrote: > yaxunl wrote: > > bader wrote: > > > yaxunl wrote: > > > > I think the default (including even_t, clk_event_t, queue_t, > > > > reserved_id_t) should be global since these opaque OpenCL objects are > > > > pointers to some shared resources. These pointers may be an automatic > > > > variable themselves but the memory they point to should be global. > > > > Since these objects are dynamically allocated, assuming them in private > > > > address space implies runtime needs to maintain a private memory pool > > > > for each work item and allocate objects from there. Considering the > > > > huge number of work items in typical OpenCL applications, it would be > > > > very inefficient to implement these objects in private memory pool. On > > > > the other hand, a global memory pool seems much reasonable. > > > > > > > > Anastasia/Alexey, any comments on this? Thanks. > > > I remember we discussed this a couple of time in the past. > > > The address space for variables of these types is not clearly stated in > > > the spec, so I think the right way to treat it - it's implementation > > > defined. > > > On the other hand your reasoning on using global address space as default > > > AS makes sense in general - so can we put additional clarification to the > > > spec to align it with the proposed implementation? > > I think it is unnecessary to specify the implementation details in the > > OpenCL spec. > > > > It is also unnecessary for SPIR-V spec since the pointee address space of > > OpenCL opaque struct is not encoded in SPIR-V. > > > > We can use the commonly accepted address space definition in the TargetInfo > > base class. If a target chooses to use different address space for specific > > opaque objects, it can override it in its own virtual function. > > I think it is unnecessary to specify the implementation details in the > > OpenCL spec. > > Agree, but my point was about specifying the intention in the specification. > For instance, OpenCL spec says that image objects are located in global > memory. > It says nothing about objects like events, queues, etc., so I assumed that if > it says nothing an implementation is allowed to choose the memory region, but > it might be false assumption. We could create a bug to Khronos OpenCL C spec and discuss it on the next call just to make sure... but otherwise this change seems reasonable enough! https://reviews.llvm.org/D33989 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34024: [OpenCL] Diagnose scoped address-space qualified variables
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM! Small comment in the test can be addressed in commit!
Comment at: test/SemaOpenCL/storageclass.cl:52
+kernel void invalidScope() {
+ if (true) {
+local int lInt; // expected-error {{variables in the local address space
can only be declared in the outermost scope of a kernel function}}
I would just move this into `foo` as it tests similar functionality already.
https://reviews.llvm.org/D34024
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34235: [OpenCL] Fix OpenCL and SPIR version metadata generation.
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D34235 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.
Anastasia added inline comments.
Comment at: test/CodeGenOpenCL/sampler.cl:62
+
+ const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE |
CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+ // CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)*
@__translate_sampler_initializer(i32 35)
bader wrote:
> yaxunl wrote:
> > what if address of const_smp is taken and assigned to a pointer to
> > sampler_t ? Do we have diagnosis in place?
> AFAIK, we have diagnostics for both:
> - declaration of a pointer to sampler
> - taking address of sampler variable
Btw, strangely I don't hit any unreachable and don't seem to have any static
variable generated either. I was trying to compile this code on r305798:
void fnc4smp(sampler_t s) {}
kernel void foo(sampler_t smp_par) {
const sampler_t const_smp = 0;
fnc4smp(const_smp);
}
This is the IR which is produced for me:
%opencl.sampler_t = type opaque
; Function Attrs: noinline nounwind optnone
define spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* %s) #0 {
entry:
%s.addr = alloca %opencl.sampler_t addrspace(2)*, align 4
store %opencl.sampler_t addrspace(2)* %s, %opencl.sampler_t addrspace(2)**
%s.addr, align 4
ret void
}
; Function Attrs: noinline nounwind optnone
define spir_kernel void @foo(%opencl.sampler_t addrspace(2)* %smp_par) #0
!kernel_arg_addr_space !4 !kernel_arg_access_qual !5 !kernel_arg_type !6
!kernel_arg_base_type !6 !kernel_arg_type_qual !7 {
entry:
%smp_par.addr = alloca %opencl.sampler_t addrspace(2)*, align 4
%const_smp = alloca %opencl.sampler_t addrspace(2)*, align 4
store %opencl.sampler_t addrspace(2)* %smp_par, %opencl.sampler_t
addrspace(2)** %smp_par.addr, align 4
%0 = call %opencl.sampler_t addrspace(2)*
@__translate_sampler_initializer(i32 0)
store %opencl.sampler_t addrspace(2)* %0, %opencl.sampler_t addrspace(2)**
%const_smp, align 4
%1 = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)**
%const_smp, align 4
call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* %1)
ret void
}
declare %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32)
https://reviews.llvm.org/D34342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin
Anastasia added inline comments.
Comment at: include/clang/Basic/Builtins.def:713
+ATOMIC_BUILTIN(__opencl_atomic_fetch_or, "v.", "t")
+ATOMIC_BUILTIN(__opencl_atomic_fetch_xor, "v.", "t")
+
What about min/max? I believe they will need to have the scope too.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6956
+ "synchronization scope argument to atomic operation is invalid">;
+def err_atomic_op_has_non_constant_synch_scope : Error<
+ "non-constant synchronization scope argument to atomic operation is not
supported">;
Btw, is this disallowed by the spec? Can't find anything relevant.
Comment at: lib/CodeGen/CGAtomic.cpp:678
RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
+ bool IsOpenCL = E->isOpenCL();
QualType AtomicTy = E->getPtr()->getType()->getPointeeType();
Seems short enough to introduce an extra variable here. :)
Comment at: lib/CodeGen/CGAtomic.cpp:707
- switch (E->getOp()) {
+ auto Op = E->getOp();
+ switch (Op) {
The same here... not sure adding an extra variable is helping here. :)
Comment at: lib/CodeGen/CGAtomic.cpp:889
+return V;
+ auto OrigLangAS = E->getType()
+.getTypePtr()
Formatting seems to be a bit odd here...
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+ "Non-constant synchronization scope not supported");
+ auto sco = (llvm::SynchronizationScope)(
+ cast(Scope)->getZExtValue());
Variable name doesn't follow the style.
Comment at: lib/CodeGen/CGAtomic.cpp:1117
+ "Non-constant synchronization scope not supported");
+ auto sco = (llvm::SynchronizationScope)(
+ cast(Scope)->getZExtValue());
Anastasia wrote:
> Variable name doesn't follow the style.
could we avoid using C style cast?
Comment at: lib/Sema/SemaChecking.cpp:3146
+Op == AtomicExpr::AO__opencl_atomic_load)
+? 0
+: 1);
formatting seems odd.
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:1
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64
-emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple
armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
GEN4 -> SPIR
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:2
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple spir64
-emit-llvm | FileCheck -check-prefix=GEN4 %s
+// RUN: %clang_cc1 < %s -cl-std=CL2.0 -finclude-default-header -triple
armv5e-none-linux-gnueabi -emit-llvm | FileCheck -check-prefix=GEN0 %s
+
GEN0 -> AMDGPU
Comment at: test/CodeGenOpenCL/atomic-ops-libcall.cl:4
+
+void f(atomic_int *i, int cmp) {
+ int x;
Could we use different scopes?
Comment at: test/CodeGenOpenCL/atomic-ops.cl:7
+
+#ifndef ALREADY_INCLUDED
+#define ALREADY_INCLUDED
why do we need this?
Comment at: test/CodeGenOpenCL/atomic-ops.cl:15
+ // CHECK: load atomic i32, i32 addrspace(4)* %{{[.0-9A-Z_a-z]+}}
singlethread seq_cst
+ int x = __opencl_atomic_load(i, memory_order_seq_cst,
memory_scope_work_item);
+}
I think we could use different scope types all over...
Comment at: test/CodeGenOpenCL/atomic-ops.cl:32
+ // CHECK-LABEL: @fi4(
+ // CHECK: [[PAIR:%[.0-9A-Z_a-z]+]] = cmpxchg i32 addrspace(4)*
[[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32
[[DESIRED:%[.0-9A-Z_a-z]+]] singlethread acquire acquire
+ // CHECK: [[OLD:%[.0-9A-Z_a-z]+]] = extractvalue { i32, i1 } [[PAIR]], 0
do we have an addrspacecast before?
Comment at: test/SemaOpenCL/atomic-ops.cl:22
+ intptr_t *P, float *D, struct S *s1, struct S *s2) {
+ __opencl_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
+ __opencl_atomic_init(ci, 5); // expected-error {{address argument to atomic
operation must be a pointer to non-const _Atomic type ('const __generic
atomic_int *' (aka 'const __generic _Atomic(int) *') invalid)}}
could we have the full error for consistency?
Comment at: test/SemaOpenCL/atomic-ops.cl:30
+ __opencl_atomic_store(i, 0, memory_order_relaxed, memory_scope_work_item);
+ __opencl_atomic_store(ci, 0, memory_order_relaxed, memory_scope_work_item);
// expected-error {{address argument to atomic operation must be a pointer to
non-const _Atomic type ('const __generic atomic_int *' (aka 'const __generic
_Atomic(int) *') invalid)}}
+
could we also test with constant AS? Also I would generally improve testing wrt
address spaces...
https://reviews.llvm.o
[PATCH] D34342: [OpenCL] Fix code generation of function-scope constant samplers.
Anastasia added a comment. In https://reviews.llvm.org/D34342#785454, @bader wrote: > Wow... > Nice catch. > For some reason I can't reproduce the problem neither. > I will drop source code change, but I'd like to modify the test anyway. I think we probably had this issue at some point... Can't think of any recent commit that fixed it apart from may be r288163. > https://reviews.llvm.org/rL277024 added test/CodeGenOpenCL/sampler.cl and > modified test/SemaOpenCL/sampler_t.cl. > I want to move the parts of the test/SemaOpenCL/sampler_t.cl, which are > supposed to pass semantic analysis to test/CodeGenOpenCL/sampler.cl and > validate the LLVM IR after code generation. > Are you OK with this? Sure. Sounds like a good improvement to testing! Thanks! https://reviews.llvm.org/D34342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.
Anastasia added inline comments.
Comment at: test/Sema/address_spaces.c:17
int *_AS1 _AS2 *Z; // expected-error {{multiple address spaces specified
for type}}
+ int *_AS1 _AS1 *M;
ebevhan wrote:
> bader wrote:
> > I think it might be valuable to give a warning or remark to user.
> > Using the same address space qualifier multiple times is not something
> > OpenCL C developers are supposed to do.
> >
> The test is obviously a bit contrived, but it could happen by mistake, or as
> a result of some typedef or macro combination. It also cannot go wrong, so
> there's no harm in it happening.
>
> I see your point, though. A warning feels like a bit much, so I'm not sure
> what else to use. A note?
Just checked for const qualifier we get a warning:
warning: duplicate 'const' declaration specifier [-Wduplicate-decl-specifier]
We could do the same... not sure if we could try to share the diagnostic as
well.
Repository:
rC Clang
https://reviews.llvm.org/D47630
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46651: [OpenCL] Support placement new/delete in Sema
Anastasia requested changes to this revision. Anastasia added inline comments. This revision now requires changes to proceed. Comment at: lib/Sema/SemaExprCXX.cpp:2030 + } +} svenvh wrote: > rjmccall wrote: > > I think a better interpretation of this rule would be to just error on > > attempts to use the standard non-placement operator new/delete instead of > > trying to outlaw the operator declarations. For example, I don't know why > > a user-defined non-global operator new would be problematic. > Good point, I have raised this with Khronos, so I will hold this off until we > have clarification. The decision by Khronos is to allow all user defined new/delete operators (even global). I have submitted the change to the spec. The next publishing date is however in July. Repository: rC Clang https://reviews.llvm.org/D46651 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
Anastasia added inline comments.
Comment at: include/clang/Basic/BuiltinsAMDGPU.def:49
+
+// FIXME: Need to disallow constant address space.
BUILTIN(__builtin_amdgcn_div_scale, "dddbb*", "n")
arsenm wrote:
> Anastasia wrote:
> > Do you plan to provide the support for it later? Or if else perhaps we
> > should elaborate more what's to be done.
> I'm not sure. I don't know how to best enforce this
The only way I guess if we list overloads with each address space explicitly
(apart from `constant`)? Or may be with the use of `generic` AS, although that
will only work for CL2.0.
Comment at: lib/AST/ASTContext.cpp:9093
unsigned AddrSpace = strtoul(Str, &End, 10);
- if (End != Str && AddrSpace != 0) {
-Type = Context.getAddrSpaceQualType(Type,
-getLangASFromTargetAS(AddrSpace));
+ if (End != Str) {
+// Note AddrSpace == 0 is not the same as an unspecified address space.
arsenm wrote:
> Anastasia wrote:
> > Could we check against LangAS::Default instead of removing this completely.
> I don't think that really make sense, since that would be leaving this the
> same. I don't really need it for this patch, but I fundamentally think
> specifying address space 0 is different from an unspecified address space.
> According to the description for builtins, if no address space is specified
> than any address space will be accepted. This is different from a builtin
> requiring address space 0
I thought `Default` AS was meant to be for the case no AS is specified but I
guess it doesn't work the same in Builtins specification syntax.
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
arsenm wrote:
> Anastasia wrote:
> > Would this be correct for OpenCL? Should we use `isAddressSpaceSupersetOf`
> > helper instead? Would it also sort the issue with constant AS (at least for
> > OpenCL)?
> The issue I mentioned for the other builtin is that it modifies the memory,
> and doesn't have to do with the casting.
>
> At this point the AddrSpaceCast has to be emitted. The checking if the cast
> is legal I guess would be in the SemaExpr part. I know at one point I was
> trying to use isAddressSpaceSupersetOf in rewriteBuiltinFunctionDecl, but
> there was some problem with that. I think it didn't make sense with the magic
> where the builtin without an address space is supposed to accept any address
> space or something along those lines.
Yes, I think Sema has to check it before indeed. I am not sure it works right
with OpenCL rules though for the Builtin functions. Would it make sense to add
a negative test for this then?
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46651: [OpenCL] Support new/delete in Sema
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D46651 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D47630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48419: [OpenCL] Fixed parsing of address spaces for C++
Anastasia created this revision.
Anastasia added a reviewer: yaxunl.
Due to missing handling of address space tokens in parsing code of C++ we were
unable to parse declarations that start from an address space keyword. For
example we would get an error:
test.cl:2:1: error: expected expression
__global int * arg_glob;
No idea if there are some more cases missing but this patch at least fixes
basic variable and function argument declaration parsing.
I enable address space test but part of it still can't run correctly in C++
mode.
https://reviews.llvm.org/D48419
Files:
lib/Parse/ParseTentative.cpp
test/SemaOpenCL/address-spaces.cl
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -cl-std=c++ -verify -pedantic -fsyntax-only
__constant int ci = 1;
@@ -8,6 +9,8 @@
__local int lj = 2; // expected-error {{'__local' variable cannot have an
initializer}}
int *ip;
+// FIXME: Temporarily disable part of the test that doesn't work for C++ yet.
+#if !__OPENCL_CPP_VERSION__
#if __OPENCL_C_VERSION__ < 200
ip = gip; // expected-error {{assigning '__global int *' to 'int *' changes
address space of pointer}}
ip = &li; // expected-error {{assigning '__local int *' to 'int *' changes
address space of pointer}}
@@ -62,4 +65,5 @@
__local __private int *var2; // expected-error {{multiple address spaces
specified for type}}
__local private_int_t var3; // expected-error {{multiple address spaces
specified for type}}
__local private_int_t *var4; // expected-error {{multiple address spaces
specified for type}}
+#endif // !__OPENCL_CXX_VERSION__
Index: lib/Parse/ParseTentative.cpp
===
--- lib/Parse/ParseTentative.cpp
+++ lib/Parse/ParseTentative.cpp
@@ -1357,6 +1357,11 @@
// cv-qualifier
case tok::kw_const:
case tok::kw_volatile:
+ case tok::kw___private:
+ case tok::kw___local:
+ case tok::kw___global:
+ case tok::kw___constant:
+ case tok::kw___generic:
// GNU
case tok::kw_restrict:
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -cl-std=c++ -verify -pedantic -fsyntax-only
__constant int ci = 1;
@@ -8,6 +9,8 @@
__local int lj = 2; // expected-error {{'__local' variable cannot have an initializer}}
int *ip;
+// FIXME: Temporarily disable part of the test that doesn't work for C++ yet.
+#if !__OPENCL_CPP_VERSION__
#if __OPENCL_C_VERSION__ < 200
ip = gip; // expected-error {{assigning '__global int *' to 'int *' changes address space of pointer}}
ip = &li; // expected-error {{assigning '__local int *' to 'int *' changes address space of pointer}}
@@ -62,4 +65,5 @@
__local __private int *var2; // expected-error {{multiple address spaces specified for type}}
__local private_int_t var3; // expected-error {{multiple address spaces specified for type}}
__local private_int_t *var4; // expected-error {{multiple address spaces specified for type}}
+#endif // !__OPENCL_CXX_VERSION__
Index: lib/Parse/ParseTentative.cpp
===
--- lib/Parse/ParseTentative.cpp
+++ lib/Parse/ParseTentative.cpp
@@ -1357,6 +1357,11 @@
// cv-qualifier
case tok::kw_const:
case tok::kw_volatile:
+ case tok::kw___private:
+ case tok::kw___local:
+ case tok::kw___global:
+ case tok::kw___constant:
+ case tok::kw___generic:
// GNU
case tok::kw_restrict:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48438: [Sema] Updated note for address spaces to print the type.
Anastasia created this revision.
Anastasia added a reviewer: yaxunl.
This allows to reuse the same diagnostic for OpenCL or CUDA.
For OpenCL this will be tested in
`test/SemaOpenCL/address-spaces-conversions-cl2.0.cl` that I plan to enable
with `-cl-std=c++` after fixing other issues.
https://reviews.llvm.org/D48438
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaOverload.cpp
test/SemaCXX/address-space-references.cpp
Index: test/SemaCXX/address-space-references.cpp
===
--- test/SemaCXX/address-space-references.cpp
+++ test/SemaCXX/address-space-references.cpp
@@ -3,10 +3,10 @@
typedef int __attribute__((address_space(1))) int_1;
typedef int __attribute__((address_space(2))) int_2;
-void f0(int_1 &); // expected-note{{candidate function not viable: 1st
argument ('int') is in address space 0, but parameter must be in address space
1}} \
-// expected-note{{candidate function not viable: 1st argument ('int_2' (aka
'__attribute__((address_space(2))) int')) is in address space 2, but parameter
must be in address space 1}}
-void f0(const int_1 &); // expected-note{{candidate function not viable: 1st
argument ('int') is in address space 0, but parameter must be in address space
1}} \
-// expected-note{{candidate function not viable: 1st argument ('int_2' (aka
'__attribute__((address_space(2))) int')) is in address space 2, but parameter
must be in address space 1}}
+void f0(int_1 &); // expected-note{{candidate function not viable: address
space mismatch in 1st argument ('int'), parameter type must be 'int_1 &' (aka
'__attribute__((address_space(1))) int &')}} \
+// expected-note{{candidate function not viable: address space mismatch in 1st
argument ('int_2' (aka '__attribute__((address_space(2))) int')), parameter
type must be 'int_1 &' (aka '__attribute__((address_space(1))) int &')}}
+void f0(const int_1 &); // expected-note{{candidate function not viable:
address space mismatch in 1st argument ('int'), parameter type must be 'const
int_1 &' (aka 'const __attribute__((address_space(1))) int &')}} \
+// expected-note{{candidate function not viable: address space mismatch in 1st
argument ('int_2' (aka '__attribute__((address_space(2))) int')), parameter
type must be 'const int_1 &' (aka 'const __attribute__((address_space(1))) int
&')}}
void test_f0() {
int i;
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -9614,9 +9614,7 @@
S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_addrspace)
<< (unsigned)FnKindPair.first << (unsigned)FnKindPair.second <<
FnDesc
<< (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
- << FromQs.getAddressSpaceAttributePrintValue()
- << ToQs.getAddressSpaceAttributePrintValue()
- << (unsigned)isObjectArgument << I + 1;
+ << ToTy << (unsigned)isObjectArgument << I + 1;
MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
return;
}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3680,8 +3680,8 @@
"%select{%ordinal4 argument|object argument}3">;
def note_ovl_candidate_bad_addrspace : Note<
"candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
-"%select{%ordinal7|'this'}6 argument (%3) is in "
-"address space %4, but parameter must be in address space %5">;
+"address space mismatch in %select{%ordinal6|'this'}5 argument (%3), "
+"parameter type must be %4">;
def note_ovl_candidate_bad_gc : Note<
"candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
"%select{%ordinal7|'this'}6 argument (%3) has %select{no|__weak|__strong}4
"
Index: test/SemaCXX/address-space-references.cpp
===
--- test/SemaCXX/address-space-references.cpp
+++ test/SemaCXX/address-space-references.cpp
@@ -3,10 +3,10 @@
typedef int __attribute__((address_space(1))) int_1;
typedef int __attribute__((address_space(2))) int_2;
-void f0(int_1 &); // expected-note{{candidate function not viable: 1st argument ('int') is in address space 0, but parameter must be in address space 1}} \
-// expected-note{{candidate function not viable: 1st argument ('int_2' (aka '__attribute__((address_space(2))) int')) is in address space 2, but parameter must be in address space 1}}
-void f0(const int_1 &); // expected-note{{candidate function not viable: 1st argument ('int') is in address space 0, but parameter must be in address space 1}} \
-// expected-note{{candidate function not viable: 1st argument ('int_2' (aka '__attribute__((address_space(2))) int')) is in address space 2, but parameter must be in address space 1}}
+vo
[PATCH] D48419: [OpenCL] Fixed parsing of address spaces for C++
Anastasia added a comment.
In https://reviews.llvm.org/D48419#1139601, @yaxunl wrote:
> Did you notice the bug that C++ silently allows implicit casting of a pointer
> to a pointer type with different address space?
>
> Do you have a fix for that?
I didn't think it did because if I run
`test/SemaOpenCL/address-spaces-conversions-cl2.0.cl` in `-cl-std=c++` it gives
me errors when I initialize a pointer with different AS pointer:
error: cannot initialize a variable of type '__global int *' with an lvalue
of type '__local int *'
error: assigning to '__global int *' from incompatible type '__local int *'
error: comparison of distinct pointer types ('__global int *' and '__local
int *')
Do you have an example code somewhere?
https://reviews.llvm.org/D48419
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
