Re: TCE target nonconforming definition of long long and intmax_t
Hi Aaron, Indeed the 32b TCE target is not fully compliant in this aspect; its 64b emulation support is not complete, therefore we advertise only these 32b limits. We have an in-progress 64b target where the limits are 64b, but it's not upstreamed yet. Yes, TCE target is still maintained, but it's a special target since it has target-specific backend phases off-tree for online retargeting and pecularities of the TTA scheduling. Thus, I'd just leave it as is for the 32b target and we'll upstream the 64b stub as soon as it stabilizes a bit. BR, Pekka On 7.12.2021 16.59, Aaron Ballman wrote: Hello! I was digging around in stdint.h to do some implementation work on C2x and I noticed that the TCE target seems to be nonconforming. In C17, the implementation limits for intmax_t and uintmax_t are specified by 7.20.2.5 as: — minimum value of greatest-width signed integer type INTMAX_MIN -(2^63 - 1) — maximum value of greatest-width signed integer type INTMAX_MAX 2^63 - 1 — maximum value of greatest-width unsigned integer type UINTMAX_MAX 2^64 - 1 Similarly, the implementation limits for long long and unsigned long long are specified by 5.2.4.2.1p1: minimum value for an object of type long long int LLONG_MIN -9223372036854775807 // -(2^63 - 1) — maximum value for an object of type long long int LLONG_MAX +9223372036854775807 // 2^63 - 1 — maximum value for an object of type unsigned long long int ULLONG_MAX 18446744073709551615 // 2^64 - 1 However, the TCE target appears to set these to 32-bit limits, not 64-bit limits: https://github.com/llvm/llvm-project/blob/2ab513cd3e0648806db7ed1f8170ad4a3d4e7749/clang/lib/Basic/Targets/TCE.h#L61 Is this target still being maintained? If so, what should be done here? (I can file a bug report about this once we have a bug database that can accept new content so we don't lose track of this.) Thanks! ~Aaron -- Pekka ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26157: [OpenCL] always use SPIR address spaces for kernel_arg_addr_space MD
pekka.jaaskelainen closed this revision. pekka.jaaskelainen added a comment. Committed in r286819. Repository: rL LLVM https://reviews.llvm.org/D26157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26157: [OpenCL] always use SPIR address spaces for kernel_arg_addr_space MD
pekka.jaaskelainen added a comment. > target architecture). So if there are no memory segments, nothing can be done > to optimize for this and therefore providing > the "fake" segment information doesn't seem to be useful? I am just trying > to understand the use case. The uses for the OpenCL logical address spaces that I know of are: 1. to differentiate local kernel arguments as their memory allocation is different (per WG if parallel WGs) 2. alias analysis: as OpenCL address spaces are per definition disjoint (no overlap), it can be utilized to prove that pointers to different address spaces are not accessing the same areas, which helps e.g. vectorization and other optimizations that require code motion/parallelization 3. clGetKernelArgInfo() implementation The kernel arg MD is good enough for 1) and 3) and helps in 2). Actually having the address spaces in the pointers would be much better, but is harder to maintain. > I am not too picky on the exact implementation details here. Perhaps well > documented hard coded numbers should work too. I am just trying to see the > use case for this and whether the current compilation flow is suboptimal to > support it properly. But perhaps also the issue is that those MDs are OpenCL > specific anyways, but the IR itself is supposed to be source language > agnostic. Yes. Maybe I should just add to the docs about the MD in my patch? I think this patch should go in regardless of a possible heavier work related to the logical address spaces as the current MD is clearly broken. Repository: rL LLVM https://reviews.llvm.org/D26157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26157: [OpenCL] always use SPIR address spaces for kernel_arg_addr_space MD
pekka.jaaskelainen added a comment. Indeed, it requires wider scale discussion to get it right, and e.g. to pass the info to AA. But to be honest, I think OpenCL and CUDA are still considered 'minority' languages in Clang/LLVM which makes me usually lean towards least intrusive implementation solutions whenever possible. The matter was discussed 5 years ago: http://clang-developers.42468.n3.nabble.com/OpenCL-Address-Spaces-and-Runtimes-td2814865.html The current situation of having the target AS in the MD is clearly not the way to go due to the loss of info for flat AS machines, so I think this discussion should be mostly about what should those designated IDs be. Sure, we could use strings such as "opencl.global" there instead, but I'm not sure how much that adds value to simply having known integer IDs instead. SPIR 1.2-2.0 is based on LLVM and designed to store info of OpenCL C kernels, therefore I thought the SPIR IDs are logical as we can just refer to SPIR specs in this case. Other alternative might be to retain the Clang's logical IDs, but I recall there was some special semantics/handling for AS IDs > 255. I might be wrong though. Repository: rL LLVM https://reviews.llvm.org/D26157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26157: [OpenCL] always use SPIR address spaces for kernel_arg_addr_space MD
pekka.jaaskelainen added a comment. In https://reviews.llvm.org/D26157#585688, @bader wrote: > @pekka.jaaskelainen, I have related question: what is the problem with > retaining OpenCL address space information in LLVM IR? > My understanding is that target CodeGen can ignore this information for the > machines with 'flat' address space model. > On the other hand I would expect this information be useful for the > Optimizer to resolve pointer aliasing. > Does it make any sense to keep OpenCL address space information in LLVM IR > generated for the machines with 'flat' address space (e.g. CPU)? Is there nowadays such a thing as "standard OpenCL logical AS IDs" which could be retained down to code gen? I must say I haven't checked the current situation here. It used to be that the logical ids are assumed to be converted to the target ones already in Clang and I'm afraid changing this requires a major rework. pocl has used the fake-address-space ids until now for exactly this -- retaining the logical AS info in the IR which can be exploited for disjoint address space AA, but is also required for handling the different AS kernel arguments as local arguments must be treated differently memory allocation wise. However, as all backends are not expected to support mapping the fake address spaces to their internal ones (in single AS it's trivial to simply ignore the AS ids, but for multi-AS machines there has to be explicit mapping) we have had an IR pass that converts the address spaces to the target's before code gen. This pass we call TargetAddressSpaces has grown way too complex and is a major source of bugs all the time. Also, another source of bugs is the fact that many passes simply ignore address spaces as they have been developed for single AS machines and only tested on them. This leads to bugs where the AS ID info is silently dropped (converted to 0) which makes them hard to catch. If the pointer creation APIs of LLVM were forced to include the AS ID in the construction, it might yield out majority of these issues -- as long as the coders respect the fact that there can be multiple ASs and not simply use 0 there all the time. Also, some optimizations such as vectorization might get confused in case it sees non-0 address spaces for CPU targets (e.g. there might not be vectorized intrinsics available for non-0 ASs). Etc. Thus, due to the limited time our group has available for hunting the bugs that stem from this, I decided it might be best to avoid the use of the "logical IDs" inside IR for now and think about how to implement the disjoint AA without them later on. Repository: rL LLVM https://reviews.llvm.org/D26157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26157: [OpenCL] always use SPIR address spaces for kernel_arg_addr_space MD
pekka.jaaskelainen added a comment. Thanks. @tstellarAMD OK to commit for 3.9.1 as well? Repository: rL LLVM https://reviews.llvm.org/D26157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20249: [OpenCL] Hierarchical/dynamic parallelism - enqueue kernel in OpenCL 2.0
pekka.jaaskelainen added a comment. LGTM, FWIW. http://reviews.llvm.org/D20249 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17596: [OpenCL] Add ocl and spir version for spir target
pekka.jaaskelainen added inline comments.
Comment at: test/CodeGenOpenCL/spir_version.cl:15
@@ +14,2 @@
+// CL20: !opencl.ocl.version = !{[[SPIR:![0-9]+]]}
+// CL20: [[SPIR]] = !{i32 2, i32 0}
Can you test 'spir64' too?
http://reviews.llvm.org/D17596
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16876: [OpenCL] Refine pipe builtin support
pekka.jaaskelainen accepted this revision. pekka.jaaskelainen added a comment. This revision is now accepted and ready to land. In http://reviews.llvm.org/D16876#359781, @Anastasia wrote: > @Pekka, do you have any more comments? Nope. Looking forward to finally implementing proper pipe support to pocl. With the future Clang OpenCL patches I appreciate if you keep adding me to the cc list of the reviews, but it might be pointless to wait for my LGTM as I'm still quite a Clang noob, thus that "LGTM" might not have the "weight" it should have :) In other words, I keep monitoring the patches and will yell if I see something that sticks to my eye, but no point in blocking the progress due to possibly slow acks from my side. http://reviews.llvm.org/D16876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16928: [OpenCL] Apply missing restrictions for Blocks in OpenCL v2.0
pekka.jaaskelainen added inline comments.
Comment at: test/SemaOpenCL/invalid-func.cl:6
@@ +5,2 @@
+void foo(int, ...); // expected-error{{OpenCL does not allow variadic
arguments}}
+int printf(const char *, ...);
I wonder should we check for the fingerprint too when letting printf() pass?
This is not the correct OpenCL printf() as the first argument should be in
constant address space.
http://reviews.llvm.org/D16928
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16686: [OpenCL] Generate metadata for opencl_unroll_hint attribute
pekka.jaaskelainen accepted this revision. pekka.jaaskelainen added a comment. LGTM. http://reviews.llvm.org/D16686 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16040: [OpenCL] Refine OpenCLImageAccessAttr to OpenCLAccessAttr
pekka.jaaskelainen accepted this revision. pekka.jaaskelainen added a comment. Otherwise LGTM. Comment at: lib/Sema/SemaChecking.cpp:267 @@ -266,3 +266,3 @@ /// Returns OpenCL access qual. // TODO: Refine OpenCLImageAccessAttr to OpenCLAccessAttr since pipe can use // it too This TODO can be removed? http://reviews.llvm.org/D16040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r258782 - Recommit: R258773 [OpenCL] Pipe builtin functions
Hi, On 01/28/2016 09:25 PM, Hans Wennborg via cfe-commits wrote: > I don't think we have a specific code owner for OpenCL in Clang, which > means Richard is the owner. FYI, in a separate thread, there's an attempt to elect Anastasia as one. -- Pekka ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15914: [OpenCL] Pipe builtin functions
pekka.jaaskelainen accepted this revision. pekka.jaaskelainen added a comment. LGTM too. Good job. You can try and ask the release manager if you can sneak it in. It's rather container patch so there might be a chance, but I don't know the policy at this stage of the release. http://reviews.llvm.org/D15914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15914: [OpenCL] Pipe builtin functions
pekka.jaaskelainen added a comment. > @Pekka, do you think it would make sense to include a description of the new > OpenCL features in the release note too? Or shall we wait for the completion > of OpenCL 2.0 support (hopefully in next release)? Could do both as well I > guess. :) Yes I think it would make sense. It's good to advertise that the OpenCL support is progressing. http://reviews.llvm.org/D15914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15914: [OpenCL] Pipe builtin functions
pekka.jaaskelainen added a comment. OK. Seems the LLVM 3.8 branching was done, it might be hard to get this in, but should we try and ask the release manager? Comment at: lib/CodeGen/CGBuiltin.cpp:1981 @@ +1980,3 @@ + const char *Name = + (BuiltinID == Builtin::BIread_pipe) ? "read_pipe_2" : "write_pipe_2"; + // Re-Creating the function type for this call, since the original type Shouldn't these have underscores, because they are not reserved OpenCL keywords/builtins? Cannot user functions be called read_pipe_2 or write_pipe_2? http://reviews.llvm.org/D15914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15914: [OpenCL] Pipe builtin functions
pekka.jaaskelainen added a comment. In http://reviews.llvm.org/D15914#322917, @pxli168 wrote: > By the way, I just got the access to the llvm svn, can I just commit the pipe > type patch directly as I see you all accepted it. Or I should send it to the > cfe-commit first? I'm afraid we won't get more reviews from cfe-commit so IMO just commit it in case it was accepted. http://reviews.llvm.org/D15914 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15603: [OpenCL] Pipe type support
pekka.jaaskelainen added inline comments. Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:108 @@ +107,3 @@ +PipeTy = llvm::PointerType::get(llvm::StructType::create( + CGM.getLLVMContext(), "opencl.pipe_t"), PipeAddrSpc); + } I'm not sure if touching the built-in fingerprints for this is a good idea. It might lead to problems and confusion. Cannot one pass pipes as arguments to user functions too? Are the fingerprints of those functions then modified accordingly? It might become messy. Because the packet size is known at host side, the pipes can be implemented as structs which holds the packet size as one of the member variables. The problem with this approach is how to exploit wider reads/writes instead of a scalar read/write loop + unpack/pack in case of vector datatypes. If the size is known only at runtime, one cannot easily generate vector reads/writes even if the element is a vector datatype and it would be efficient to keep the packet in a vector register as long as possible. For helping this I'd add a metadata which can be utilized at compile time to make reading/writing from the pipe faster. But in a way that is already an optimization, not a requirement, to make pipes work. The reading itself is platform dependent as the pipe can be even a hardware FIFO accessed using special instructions. http://reviews.llvm.org/D15603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15603: [OpenCL] Pipe type support
pekka.jaaskelainen added inline comments. Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:108 @@ +107,3 @@ +PipeTy = llvm::PointerType::get(llvm::StructType::create( + CGM.getLLVMContext(), "opencl.pipe_t"), PipeAddrSpc); + } pxli168 wrote: > pekka.jaaskelainen wrote: > > I'm not sure if touching the built-in fingerprints for this is a good idea. > > It might lead to problems and confusion. Cannot one pass pipes as arguments > > to user functions too? Are the fingerprints of those functions then > > modified accordingly? It might become messy. > > > > Because the packet size is known at host side, the pipes can be implemented > > as structs which holds the packet size as one of the member variables. The > > problem with this approach is how to exploit wider reads/writes instead of > > a scalar read/write loop + unpack/pack in case of vector datatypes. > > > > If the size is known only at runtime, one cannot easily generate vector > > reads/writes even if the element is a vector datatype and it would be > > efficient to keep the packet in a vector register as long as possible. For > > helping this I'd add a metadata which can be utilized at compile time to > > make reading/writing from the pipe faster. But in a way that is already an > > optimization, not a requirement, to make pipes work. > > > > The reading itself is platform dependent as the pipe can be even a hardware > > FIFO accessed using special instructions. > This is what I'm worry about, I don't think we need to give much information > about an opaque type in OpenCL. > > Actually we can get the element type from the metadata, and I think we can > leave the optimization to the backend and let platform to choose which way to > use for read/write pipe. > > And I think the built-in function support for the pipe in OpenCL-C is not > necessary in the clang, what do you think? Though it can do some check in > Sema check, they can also be done in some llvm pass in backend. If the > built-in is really needed, I will send another patch based on this included > built-in support for pipe. > > Thank you. As far as I've understood, no there's no need to add built-in function awareness to the frontend for this case. Sema checking/diagnostics is needed to ensure pipes are used only as function arguments, not local variables, for example. Is this patch missing it? http://reviews.llvm.org/D15603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [Patch][OpenCL] Custom atomic Builtin check ignores address space of a non-atomic pointer
Hi Anastasia,
Still LGTM.
Pekka
On 16.12.2015 16:42, Anastasia Stulova wrote:
Hi Pekka,
Re-attaching as a full patch again as something went wrong earlier with the
diff wrapping in the email.
Thanks,
Anastasia
-Original Message-
From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
Anastasia Stulova via cfe-commits
Sent: 07 December 2015 17:25
To: 'Pekka Jääskeläinen'; cfe-commits@lists.llvm.org
Subject: RE: [Patch][OpenCL] Custom atomic Builtin check ignores address space
of a non-atomic pointer
Could someone jump in, please!
I have made a small improvement in testing after the patch has been first
approved by Pekka.
The last change is essentially this:
Index: test/CodeGen/atomic-ops.c
===
--- test/CodeGen/atomic-ops.c (revision 250025)
+++ test/CodeGen/atomic-ops.c (working copy)
@@ -1,10 +1,10 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding
-triple=i686-apple-darwin9 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding
+-ffake-address-space-map -triple=i686-apple-darwin9 | FileCheck %s
// REQUIRES: x86-registered-target
// Also test serialization of atomic operations here, to avoid duplicating
the // test.
-// RUN: %clang_cc1 %s -emit-pch -o %t -ffreestanding
-triple=i686-apple-darwin9 -// RUN: %clang_cc1 %s -include-pch %t
-ffreestanding -triple=i686-apple-darwin9 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-pch -o %t -ffreestanding
+-ffake-address-space-map -triple=i686-apple-darwin9 // RUN: %clang_cc1
+%s -include-pch %t -ffreestanding -ffake-address-space-map
+-triple=i686-apple-darwin9 -emit-llvm -o - | FileCheck %s
#ifndef ALREADY_INCLUDED
#define ALREADY_INCLUDED
@@ -155,6 +155,14 @@
return atomic_compare_exchange_strong(i, , 1); }
+#define _AS1 __attribute__((address_space(1))) _Bool fi4d(_Atomic(int)
+*i, int _AS1 *ptr2) {
+ // CHECK-LABEL: @fi4d(
+ // CHECK: [[EXPECTED:%[.0-9A-Z_a-z]+]] = load i32, i32 addrspace(1)*
+%{{[0-9]+}}
+ // CHECK: cmpxchg i32* %{{[0-9]+}}, i32 [[EXPECTED]], i32 %{{[0-9]+}}
+acquire acquire
+ return __c11_atomic_compare_exchange_strong(i, ptr2, 1,
+memory_order_acquire, memory_order_acquire); }
+
float ff1(_Atomic(float) *d) {
// CHECK-LABEL: @ff1
// CHECK: load atomic i32, i32* {{.*}} monotonic
Thank you!
Anastasia
-Original Message-
From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
Anastasia Stulova via cfe-commits
Sent: 23 November 2015 10:32
To: cfe-commits@lists.llvm.org; 'Pekka Jääskeläinen'
Subject: RE: [Patch][OpenCL] Custom atomic Builtin check ignores address space
of a non-atomic pointer
Ping! Re-attaching the final patch.
-Original Message-
From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
Anastasia Stulova via cfe-commits
Sent: 21 October 2015 11:49
To: 'Pekka Jääskeläinen'; cfe-commits@lists.llvm.org
Subject: RE: [Patch][OpenCL] Custom atomic Builtin check ignores address space
of a non-atomic pointer
Hi Pekka,
Are you ok with this change?
Thanks,
Anastasia
-Original Message-
From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
Anastasia Stulova via cfe-commits
Sent: 12 October 2015 16:00
To: 'Pekka Jääskeläinen'; cfe-commits@lists.llvm.org
Subject: RE: [Patch][OpenCL] Custom atomic Builtin check ignores address space
of a non-atomic pointer
I have just made one minor update in the CodeGen test
(test/CodeGen/atomic-ops.c) that is now checking the IR output rather than only
making sure frontend doesn't crash.
The final patch is attached here!
Thanks,
Anastasia
-Original Message-
From: Pekka Jääskeläinen [mailto:pekka.jaaskelai...@tut.fi]
Sent: 02 October 2015 10:20
To: Anastasia Stulova; cfe-commits@lists.llvm.org
Subject: Re: [Patch][OpenCL] Custom atomic Builtin check ignores address space
of a non-atomic pointer
LGTM.
Related to it:
There has been so many getPointerTo() issues with multi-AS in the past that I
wonder if it'd be time to drop the default value from it, and go through all
the places where it's called with the default AS, thus breaking multi-AS.
Might be a worthwhile job to do at some point.
On 09/30/2015 01:23 PM, Anastasia Stulova via cfe-commits wrote:
Hi all,
Address spaces are not handled in custom semantic checks of atomic Builtins.
If there are two pointers passed to the Builtin, it doesn't allow the
second
(non-atomic) one to be qualified with an address space.
This patch removed this restriction by recording the address space of
the
passed pointers while checking its type correctness.
Currently, the following code:
_Atomic int __attribute__((address_space(1))) *A;
int __attribute__((address_space(2))) *B;
...
... = __c11_atomic_compare_exchange_strong(A, B, 1,
memory_order_seq_cst, memory_order_seq_cst);
fails to compile with an error:
"passing '__attribute__((address_space(2))) int *' to parameter of
Re: [PATCH] D14441: [OpenCL] Pipe types support.
pekka.jaaskelainen added a comment. Is this still waiting for some updates? http://reviews.llvm.org/D14441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14441: [OpenCL] Pipe types support.
pekka.jaaskelainen accepted this revision. This revision is now accepted and ready to land. Comment at: include/clang/Serialization/ASTBitCodes.h:911 @@ +910,3 @@ + TYPE_ADJUSTED = 42, + /// \brief An PipeType record. + TYPE_PIPE = 43 Sorry. I meant 'A PipeType' instead of 'An PipeType' :) http://reviews.llvm.org/D14441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14441: [OpenCL] Pipe types support.
pekka.jaaskelainen requested changes to this revision.
pekka.jaaskelainen added a comment.
This revision now requires changes to proceed.
Only small issues found, plus it could use some more test cases.
Comment at: include/clang/AST/TypeLoc.h:2049
@@ +2048,3 @@
+ SourceRange getLocalSourceRange() const {
+return SourceRange( getKWLoc() );
+ }
Extra white spaces.
Comment at: include/clang/Sema/DeclSpec.h:1425
@@ -1415,1 +1424,3 @@
+struct PipeTypeInfo : TypeInfoCommon {
+/// The access writes.
Indent error.
Comment at: include/clang/Sema/DeclSpec.h:1426
@@ +1425,3 @@
+struct PipeTypeInfo : TypeInfoCommon {
+/// The access writes.
+unsigned AccessWrites : 3;
I think this needs a better comment than duplicating the variable name. What is
the purpose of the variable?
Comment at: include/clang/Sema/DeclSpec.h:1554
@@ +1553,3 @@
+ static DeclaratorChunk getPipe(unsigned TypeQuals,
+ SourceLocation Loc) {
+DeclaratorChunk I;
Indent issue.
Comment at: include/clang/Serialization/ASTBitCodes.h:911
@@ +910,3 @@
+ TYPE_ADJUSTED = 42,
+ /// \brief An PipeType record.
+ TYPE_PIPE = 43
A
Comment at: lib/AST/ASTContext.cpp:3121
@@ +3120,3 @@
+/// getPipeType - Return pipe type for the specified type.
+QualType ASTContext::getPipeType(QualType T ) const {
+ // Unique pointers, to guarantee there is only one pointer of a particular
White space errror.
Comment at: lib/AST/ASTContext.cpp:3141
@@ +3140,3 @@
+ }
+ PipeType *New = new (*this, TypeAlignment) PipeType(T, Canonical);
+ Types.push_back(New);
Should we assign T to 'Canonical' if it already was Canonical or is this
intentional somehow?
Comment at: lib/AST/ItaniumMangle.cpp:2672
@@ +2671,3 @@
+ // ::= U # vendor extended type qualifier
+ // (Until there's a standardized mangling...)
+ Out << "8ocl_pipe";
This seems to conform to SPIR 2.0, should we mention it here?
Comment at: lib/Parse/ParseDecl.cpp:4954
@@ +4953,3 @@
+
+ if ( D.getDeclSpec().isTypeSpecPipe() ) {
+DeclSpec = D.getMutableDeclSpec();
WS issues.
Comment at: lib/Parse/ParseDecl.cpp:4958
@@ +4957,3 @@
+ D.AddTypeInfo(DeclaratorChunk::getPipe(DS.getTypeQualifiers(),
+ DS.getPipeLoc()),
+ DS.getAttributes(),
Indent issues.
Comment at: lib/Sema/DeclSpec.cpp:729
@@ +728,3 @@
+
+ if ( TypeSpecType != TST_unspecified ) {
+PrevSpec = DeclSpec::getSpecifierName((TST) TypeSpecType, Policy);
WS issues.
Comment at: lib/Sema/DeclSpec.cpp:735
@@ +734,3 @@
+
+ if ( isPipe ) {
+TypeSpecPipe = TSP_pipe;
ditto
Comment at: lib/Sema/SemaType.cpp:1570
@@ -1564,3 +1569,3 @@
// attributes are pushed around.
- if (AttributeList *attrs = DS.getAttributes().getList())
-processTypeAttrs(state, Result, TAL_DeclSpec, attrs);
+ if ( !DS.isTypeSpecPipe() ) { // pipe attributes will be handled
later ( at GetFullTypeForDeclarator )
+if ( AttributeList *attrs = DS.getAttributes().getList())
WS issues and an overlong line.
Comment at: lib/Sema/SemaType.cpp:1571
@@ +1570,3 @@
+ if ( !DS.isTypeSpecPipe() ) { // pipe attributes will be handled
later ( at GetFullTypeForDeclarator )
+if ( AttributeList *attrs = DS.getAttributes().getList())
+ processTypeAttrs(state, Result, TAL_DeclSpec, attrs);
WS issues.
Comment at: lib/Sema/SemaType.cpp:1945
@@ +1944,3 @@
+QualType Sema::BuildPipeType(QualType T,
+SourceLocation Loc) {
+ assert(!T->isObjCObjectType() && "Should build ObjCObjectPointerType");
Indent
Comment at: lib/Sema/TreeTransform.h:5254
@@ +5253,3 @@
+QualType TreeTransform::TransformPipeType(TypeLocBuilder ,
+ PipeTypeLoc TL) {
+ QualType ValueType = getDerived().TransformType(TLB, TL.getValueLoc());
Indent
Comment at: test/CodeGenOpenCL/pipe_types.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+
Can you add a couple of more checks? For example:
- the behavior when using CL1.2
- when the inner type of the pipe has a 'const' or similar qualifier
- It can only by a function arg. What if it's a local variable?
http://reviews.llvm.org/D14441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Re: [PATCH] D13349: [OpenCL] Casting boolean to an integer vector in OpenCL should set all bits if boolean is true
pekka.jaaskelainen added a comment. OK now. http://reviews.llvm.org/D13349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [Patch][OpenCL] Custom atomic Builtin check ignores address space of a non-atomic pointer
LGTM. Related to it: There has been so many getPointerTo() issues with multi-AS in the past that I wonder if it'd be time to drop the default value from it, and go through all the places where it's called with the default AS, thus breaking multi-AS. Might be a worthwhile job to do at some point. On 09/30/2015 01:23 PM, Anastasia Stulova via cfe-commits wrote: Hi all, Address spaces are not handled in custom semantic checks of atomic Builtins. If there are two pointers passed to the Builtin, it doesn't allow the second (non-atomic) one to be qualified with an address space. This patch removed this restriction by recording the address space of the passed pointers while checking its type correctness. Currently, the following code: _Atomic int __attribute__((address_space(1))) *A; int __attribute__((address_space(2))) *B; ... ... = __c11_atomic_compare_exchange_strong(A, B, 1, memory_order_seq_cst, memory_order_seq_cst); fails to compile with an error: "passing '__attribute__((address_space(2))) int *' to parameter of type 'int *' changes address space of pointer". Please, review the attached fix for it! Cheers, Anastasia ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits -- Pekka ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13168: [OpenCL] OpenCL2.0 - Apply default address space rules
pekka.jaaskelainen requested changes to this revision. This revision now requires changes to proceed. Comment at: lib/Sema/SemaType.cpp:6166 @@ -6166,2 +6165,3 @@ attr.setUsedAsTypeAttr(); + hasOpenCLAddressSpace = true; break; Should this be below AttributeList::AT_OpenCLGenericAddressSpace? Does it now include the address space attribute even when used from C? Probably doesn't harm, but a bit misleading. Comment at: lib/Sema/SemaType.cpp:6282 @@ +6281,3 @@ +} else if (state.getCurrentChunkIndex() == 0 && + D.getContext() == Declarator::FileContext && + !D.isFunctionDeclarator() && !D.isFunctionDefinition() && Should this "white list" the cases where it's safe to add the default AS, not assume that it's applicable to any not listed here? Maybe safer that way. http://reviews.llvm.org/D13168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D10586: Add HSAIL support
pekka.jaaskelainen added a comment. Is there a matching HLC-HSAIL branch for the latest version of the patch? It doesn't work with with hsail-stable-3.7 anymore. http://reviews.llvm.org/D10586 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12378: [OpenCL] Improve diagnostics detecting implicit vector conversion.
pekka.jaaskelainen added a subscriber: pekka.jaaskelainen. pekka.jaaskelainen accepted this revision. pekka.jaaskelainen added a reviewer: pekka.jaaskelainen. pekka.jaaskelainen added a comment. This revision is now accepted and ready to land. Perhaps the assert() is unneccessary, but otherwise LGTM. http://reviews.llvm.org/D12378 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
