Initial support for OpenCL 2.0 feature: pipe types.
Details
Diff Detail
Event Timeline
Only small issues found, plus it could use some more test cases.
include/clang/AST/TypeLoc.h | ||
---|---|---|
32 | Extra white spaces. | |
include/clang/Sema/DeclSpec.h | ||
23 | Indent error. | |
24 | I think this needs a better comment than duplicating the variable name. What is the purpose of the variable? | |
25 | Indent issue. | |
include/clang/Serialization/ASTBitCodes.h | ||
3 | A | |
lib/AST/ASTContext.cpp | ||
58 | White space errror. | |
78 | Should we assign T to 'Canonical' if it already was Canonical or is this intentional somehow? | |
lib/AST/ItaniumMangle.cpp | ||
12 | This seems to conform to SPIR 2.0, should we mention it here? | |
lib/Parse/ParseDecl.cpp | ||
27 | WS issues. | |
31 | Indent issues. | |
lib/Sema/DeclSpec.cpp | ||
25 | WS issues. | |
31 | ditto | |
lib/Sema/SemaType.cpp | ||
53–1 | WS issues and an overlong line. | |
54 | WS issues. | |
65 | Indent | |
lib/Sema/TreeTransform.h | ||
34 | Indent | |
test/CodeGenOpenCL/pipe_types.cl | ||
2 | Can you add a couple of more checks? For example:
|
test/CodeGenOpenCL/pipe_types.cl | ||
---|---|---|
6 | Great work!! |
Applied code review comments.
include/clang/Sema/DeclSpec.h | ||
---|---|---|
24 | It looks like it was added to handle pipe access qualifiers, but as far as I can see it doesn't do anything useful right now. I'll remove it. | |
include/clang/Serialization/ASTBitCodes.h | ||
3 | No sure I understand that comment. Pekka, could you clarify, please? | |
lib/AST/ASTContext.cpp | ||
78 | My understanding is that it's not required. T is used in most cases and 'Canonical' is queried only if T is not canonical. I looked at the similar methods of ASTContext - ASTContext::get*Type - they all pass default constructed 'Canonical' if original type is canonical. | |
test/CodeGenOpenCL/pipe_types.cl | ||
2 | I've added negative test that pipe types are not complied with CL1.2, but I'd like to note that this not the final patch. I'm also going to commit support for pipe built-ins and semantic checks as a separate patches. | |
6 | I've added a test case for vector element pipes. It works for me. Could you send me you reproducer, please? |
include/clang/Serialization/ASTBitCodes.h | ||
---|---|---|
911 | Sorry. I meant 'A PipeType' instead of 'An PipeType' :) |
This is the log of runing
clang -cc1 -emit-llvm -cl-std=CL2.0 -o - pipe_types.cl
with the minor changed pipe_types.cl
test/CodeGenOpenCL/pipe_types.cl | ||
---|---|---|
7 | I find the bug is really strange, I just changed the int here into int4 and the clang is broken(I will attach the log). But if int4 shows below some function with arg like pipe int, it will be alright. |
include/clang/AST/Type.h | ||
---|---|---|
5018 | Any reason for an empty comment line here? | |
lib/AST/ASTContext.cpp | ||
1848 | Break not needed? | |
lib/AST/ItaniumMangle.cpp | ||
2673 | May be a reference to a section in the spec? | |
lib/CodeGen/CGDebugInfo.cpp | ||
2005 | what does "atomic wrapping" mean here? | |
2006 | Also I don't get this comment. But I see that it's copied from atomic. | |
lib/Sema/SemaType.cpp | ||
1941 | Is "pointer type" correct here? | |
4146 | Why this brace here? |
include/clang/AST/Type.h | ||
---|---|---|
5019 | Not related to this change though, but I was just thinking that it might be possible to avoid a lot of code repetition by moving common functionality of most special type classes into a base class. For example, AtomicType, BlockPointerType, ComplexType, PointerType, PipeType all share similar functionality being some sort of a wrapper/container of another type. It will also avoid a lot of code replication in ASTContext (for example get{Atomic|BlockPointer|Pointer|Complex|Pipe}Type methods) as we could have some common factory pattern like template method for creating these types and filling the type caches. It seems they are mostly done in similar way. Would be a nice refactoring task I think. |
Hi all,
Bader had asked me to help him finished the patch set supporting the OpenCL2.0. His original work is based on llvm 3.6.2, and include almost everything needed for 2.0. Now I am working on the rest of the pipe support, include the full support to complex types and built-in functions. I will update the patch as soon as I finished re-basing and having a simple test.
Thank you.
Hi guys,
Sorry for long silence.
Unfortunately, I just can't find time to finish with this patch.
Xiuli provided a valid test case that exposed issue with type caching, so I decided to not commit at as is (although I applied other minor style comments locally).
I'm currently working on front-end part of OpenCL C -> SPIR-V compiler, which is developed on github: https://github.com/KhronosGroup/SPIR/tree/spirv-1.0. It should be fully OpenCL C 1.x and 2.0 conformant clang.
I would be really happy to see patches from that branches to be in the llvm.org repository and I would appreciate any help with porting them from 3.6 release branch.
Sorry for inconvenience,
Alexey Bader
Hi guys,
I have upload a the bugfix version for bader's patch, but I could not merge my patch to his. It automatically became a new diff http://reviews.llvm.org/D15603.
I will upload the pipe related built-in and some bug fix patches after this is merged, the alltogether patch is too huge.
Thanks
Xiuli
Any reason for an empty comment line here?