Page MenuHomePhabricator

[OpenCL] Pipe types support.
AbandonedPublic

Authored by bader on Nov 6 2015, 7:07 AM.

Details

Summary

Initial support for OpenCL 2.0 feature: pipe types.

Diff Detail

Event Timeline

bader updated this revision to Diff 39521.Nov 6 2015, 7:07 AM
bader retitled this revision from to [OpenCL] Pipe types support..
bader updated this object.
bader added reviewers: pekka.jaaskelainen, gbenyei.
bader added a subscriber: cfe-commits.
pekka.jaaskelainen requested changes to this revision.Nov 10 2015, 9:07 AM
pekka.jaaskelainen edited edge metadata.

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:

  • 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?
This revision now requires changes to proceed.Nov 10 2015, 9:07 AM
pxli168 added inline comments.
test/CodeGenOpenCL/pipe_types.cl
6

Great work!!
But I have tried your patch and find it does not support opencl gentype like int4 with the usual used typedef in other test cases.
Maybe there is something wrong?

bader updated this revision to Diff 40058.Nov 12 2015, 9:07 AM
bader edited edge metadata.
bader marked 13 inline comments as done.

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?

pekka.jaaskelainen edited edge metadata.
pekka.jaaskelainen added inline comments.
include/clang/Serialization/ASTBitCodes.h
911

Sorry. I meant 'A PipeType' instead of 'An PipeType' :)

This revision is now accepted and ready to land.Nov 12 2015, 11:49 AM
pxli168 added a comment.EditedNov 12 2015, 11:59 PM


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.
Hope the log may help.

Anastasia added inline comments.
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?

Anastasia added inline comments.Nov 23 2015, 6:37 AM
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.

Is this still waiting for some updates?

I've made a few small comments. Don't see any reply though.

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.

bader added a comment.Dec 15 2015, 3:23 AM

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

Nice! I will keep on trying to help by reviewing patches whenever time allows.

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

bader abandoned this revision.Dec 16 2015, 11:56 PM

New version of this patch was uploaded by pxli168 at http://reviews.llvm.org/D15603.