[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

This revision was automatically updated to reflect the committed changes. Closed by commit rL315668: [OpenCL] Add LangAS::opencl_private to represent private address space in AST (authored by yaxunl). Changed prior to commit: https://reviews.llvm.org/D35082?vs=118813=118882#toc Repository:

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. A few minor comments; feel free to commit after addressing them. Comment at: include/clang/Basic/AddressSpaces.h:34 // OpenCL specific address spaces. opencl_global, yaxunl wrote: > rjmccall wrote: > > I think you need a real

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 118813. yaxunl marked 7 inline comments as done. yaxunl edited the summary of this revision. yaxunl added a comment. Separate implicit addr space flag to another patch as John suggested. This patch only introduces the private addr space but does not print it.

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. In https://reviews.llvm.org/D35082#895312, @yaxunl wrote: > Thanks. I will separate the implicit addr space flag to another patch. Thanks, appreciated. Comment at: include/clang/AST/Type.h:562 + static const uint32_t IMask = 0x200; + static const

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 2 inline comments as done. yaxunl added a comment. In https://reviews.llvm.org/D35082#895230, @rjmccall wrote: > It sounds like there's agreement about the basic technical direction of > introducing LangAS::opencl_private. Please introduce > isAddressSpaceImplicit() in a

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. It sounds like there's agreement about the basic technical direction of introducing LangAS::opencl_private. Please introduce isAddressSpaceImplicit() in a different patch and make this patch just about the introduction of LangAS::opencl_private. You can have the

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl added a comment. If there is no other issues. May I commit this patch now? Thanks. 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] 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

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. In https://reviews.llvm.org/D35082#890359, @Anastasia wrote: > 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

[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

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. 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

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl added a comment. In https://reviews.llvm.org/D35082#890150, @rjmccall wrote: > Non-canonical information is not supposed to be mangled. > > It's not clear to me what the language rule you're really trying to implement > is. Maybe you really do need a canonical __private address space,

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. Non-canonical information is not supposed to be mangled. It's not clear to me what the language rule you're really trying to implement is. Maybe you really do need a canonical __private address space, in which case you are going to have to change a lot of code in

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl added a comment. In https://reviews.llvm.org/D35082#890143, @rjmccall wrote: > You have an important backend change relying on being able to preserve type > sugar better in diagnostics? The only apparent semantic change in this patch > is that you're changing the mangling, which

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. You have an important backend change relying on being able to preserve type sugar better in diagnostics? The only apparent semantic change in this patch is that you're changing the mangling, which frankly seems incorrect. https://reviews.llvm.org/D35082

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl added a comment. In https://reviews.llvm.org/D35082#889053, @rjmccall wrote: > Are you sure it's a good idea to not print the address space when it's > implicit? Won't that often lead to really confusing diagnostics? > > Also, we do already have a way of expressing that an extended

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. Are you sure it's a good idea to not print the address space when it's implicit? Won't that often lead to really confusing diagnostics? Also, we do already have a way of expressing that an extended qualifier was explicit: AttributedType. We have very similar sorts

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl added a comment. In https://reviews.llvm.org/D35082#887855, @rjmccall wrote: > Why is most of this patch necessary under the design of adding a > non-canonical __private address space? There are two reasons that we need a flag to indicate an address space is simplicit: 1. We need a

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 117770. yaxunl marked 9 inline comments as done. yaxunl added a comment. Revised by John's comments. https://reviews.llvm.org/D35082 Files: include/clang/AST/ASTContext.h include/clang/AST/Type.h include/clang/Basic/AddressSpaces.h

[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

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. Why is most of this patch necessary under the design of adding a non-canonical __private address space? Comment at: include/clang/AST/Type.h:336 + /// space makes difference. + bool getImplicitAddressSpaceFlag() const { return Mask & IMask; } +

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 117020. yaxunl marked 3 inline comments as done. yaxunl added a comment. Revised by Anastasia's comments. https://reviews.llvm.org/D35082 Files: include/clang/AST/ASTContext.h include/clang/AST/Type.h include/clang/Basic/AddressSpaces.h

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 5 inline comments as done. yaxunl added inline comments. Comment at: lib/Sema/SemaType.cpp:6810 + QualType , TypeAttrLocation TAL) { + if (!State.getSema().getLangOpts().OpenCL || + T.getAddressSpace() !=

[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 , + QualType , TypeAttrLocation TAL) { Great! This looks so clear now!

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: lib/Sema/SemaType.cpp:6974 + if (state.getSema().getLangOpts().OpenCL && !hasOpenCLAddressSpace && + type.getAddressSpace() == LangAS::Default && Anastasia wrote: > I am

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 116704. yaxunl marked 5 inline comments as done. yaxunl added a comment. Rebase to ToT and clean up logic. https://reviews.llvm.org/D35082 Files: include/clang/AST/ASTContext.h include/clang/AST/Type.h include/clang/Basic/AddressSpaces.h

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

Anastasia added inline comments. Comment at: lib/Sema/SemaType.cpp:6994 + // OpenCL v1.2 s6.5: + // The generic address space name for arguments to a function in a + // program, or local variables of a function is __private. All function yaxunl

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: lib/Sema/SemaType.cpp:6994 + // OpenCL v1.2 s6.5: + // The generic address space name for arguments to a function in a + // program, or local variables of a function is __private.

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

Anastasia added inline comments. Comment at: lib/Sema/SemaType.cpp:6994 + // OpenCL v1.2 s6.5: + // The generic address space name for arguments to a function in a + // program, or local variables of a function is __private. All function yaxunl

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 114337. yaxunl marked 3 inline comments as done. yaxunl edited the summary of this revision. yaxunl added a comment. Add comments for getImplicitAddressSpaceFlag and fix checking of null pointer. https://reviews.llvm.org/D35082 Files:

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 6 inline comments as done. yaxunl added inline comments. Comment at: include/clang/AST/Type.h:332 + bool getImplicitAddressSpaceFlag() const { return Mask & IMask; } + void setImplicitAddressSpaceFlag(bool Value) { Anastasia wrote: > Could we

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

Anastasia added inline comments. Comment at: include/clang/AST/Type.h:332 + bool getImplicitAddressSpaceFlag() const { return Mask & IMask; } + void setImplicitAddressSpaceFlag(bool Value) { Could we add a bit of comment somewhere explaining why we added

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked an inline comment as done. yaxunl added a comment. ping 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] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11846 // an address space. if (T.getAddressSpace() != 0) { // OpenCL allows function arguments declared to be an array of a type yaxunl wrote: > Anastasia wrote: > > yaxunl wrote: > > >

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 6 inline comments as done. yaxunl added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11846 // an address space. if (T.getAddressSpace() != 0) { // OpenCL allows function arguments declared to be an array of a type Anastasia wrote:

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 112288. yaxunl edited the summary of this revision. yaxunl added a comment. Herald added subscribers: nhaehnle, jholewinski. Add a flag to indicate whether address space qualifier is implicit and only print explicit address space in diagnostics.

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. Well, we don't currently have a concept of a non-canonical qualifier, but I think it probably wouldn't be difficult to support; you would just need ASTContext::getQualifiedType to be aware of it. https://reviews.llvm.org/D35082

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl added a comment. In https://reviews.llvm.org/D35082#844620, @rjmccall wrote: > The meaning we've agreed on for LangAS::Default is to be the address space of > local declarations, which corresponds quite well to __private in OpenCL. I > think your concern about diagnostics is better

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. The meaning we've agreed on for LangAS::Default is to be the address space of local declarations, which corresponds quite well to __private in OpenCL. I think your concern about diagnostics is better addressed by changing the pretty-printer than by changing Sema to

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: lib/Sema/SemaType.cpp:6969 + if (state.getSema().getLangOpts().OpenCL && + !hasOpenCLAddressSpace && type.getAddressSpace() == 0 && Anastasia wrote: > yaxunl wrote: > >

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

Anastasia added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11846 // an address space. if (T.getAddressSpace() != 0) { // OpenCL allows function arguments declared to be an array of a type yaxunl wrote: > Anastasia wrote: > > yaxunl wrote: > >

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 6 inline comments as done. yaxunl added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11846 // an address space. if (T.getAddressSpace() != 0) { // OpenCL allows function arguments declared to be an array of a type Anastasia wrote:

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

Anastasia added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11846 // an address space. if (T.getAddressSpace() != 0) { // OpenCL allows function arguments declared to be an array of a type yaxunl wrote: > Anastasia wrote: > > Could we use

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 106486. yaxunl marked 6 inline comments as done. yaxunl added a comment. Revised by Anastasia's comments. https://reviews.llvm.org/D35082 Files: include/clang/Basic/AddressSpaces.h lib/AST/ASTContext.cpp lib/AST/Expr.cpp lib/AST/ItaniumMangle.cpp

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl marked 6 inline comments as done. yaxunl added inline comments. Comment at: lib/AST/Expr.cpp:3235 +// has non-default address space it is not treated as nullptr. +bool PointeeHasDefaultAS = false; +if (Ctx.getLangOpts().OpenCL)

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

Anastasia added inline comments. Comment at: lib/AST/Expr.cpp:3235 +// has non-default address space it is not treated as nullptr. +bool PointeeHasDefaultAS = false; +if (Ctx.getLangOpts().OpenCL) Would we still be accepting the

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

bader accepted this revision. bader added a comment. This revision is now accepted and ready to land. LGTM, thanks. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 105785. yaxunl added a comment. Treat (void*)0 as null pointer for OpenCL 1.2. Rebase to ToT. https://reviews.llvm.org/D35082 Files: include/clang/Basic/AddressSpaces.h lib/AST/ASTContext.cpp lib/AST/Expr.cpp lib/AST/ItaniumMangle.cpp

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

yaxunl updated this revision to Diff 105774. yaxunl retitled this revision from "[WIP][OpenCL] Add LangAS::opencl_private to represent private address space in AST" to "[OpenCL] Add LangAS::opencl_private to represent private address space in AST". yaxunl edited the summary of this revision.