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 `LangAS::Default` here too?
> done
Sorry, I wasn't clear. I think we could have:
if (T.getAddressSpace() != LangAS::Default && T.getAddressSpace() !=
LangAS::opencl_private)
and then original condition. It is a bit clearer I think.
================
Comment at: lib/Sema/SemaType.cpp:6969
+ if (state.getSema().getLangOpts().OpenCL &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
----------------
yaxunl wrote:
> Anastasia wrote:
> > Would it be nicer to not append any address space at all neither here nor
> > down at the end of this function and keep it default instead until the
> > Codegen? If it's doable I would very much prefer that. It seems like it
> > would make the implementation potentially a bit cleaner to understand and
> > easier to improve semantical analysis. See one example of improving
> > original type printing in my comment to the test below.
> >
> > Also there are at least these 3 related bugs open currently:
> > https://bugs.llvm.org//show_bug.cgi?id=33418
> > https://bugs.llvm.org//show_bug.cgi?id=33419
> > https://bugs.llvm.org//show_bug.cgi?id=33420
> >
> > Does your change address any of those?
> On the contrary, I think using default address space for automatic variable
> and function parameter will cause ambiguity and inconsistency in AST, making
> it more difficult to understand and process, and making some bug (e.g.
> https://bugs.llvm.org//show_bug.cgi?id=33419) unfixable. For example,
> `private int f(void)` and `int f(void)` will be identical in AST, therefore
> we cannot diagnose `private int f(void)`.
>
> With current representation I am able to fix the 3 bugs. I will update the
> diff.
I don't see why?
`private int f(void)` -> will have an address space attribute in AST as it is
provided explicitly.
`int f(void) ` -> will have no address space attribute because it's not
provided explicitly and not attached implicitly either.
All I was asking is not to deduce the address space here if it's not specified
explicitly until later step when we need to put it in the IR.
================
Comment at: test/SemaOpenCL/access-qualifier.cl:23
#else
-void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__read_write image1d_t' prior to OpenCL
version 2.0}}
+void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__private __read_write image1d_t' prior to
OpenCL version 2.0}}
#endif
----------------
yaxunl wrote:
> Anastasia wrote:
> > Ok, I think that here it would be less confusing not to add any address
> > space since it's missing in the original source.
> Although it looks strange at first sight, `__private __read_write image1d_t`
> is the true type of the argument. The function argument is allocated from
> stack and is an l-value. If we take its address, we get a private pointer.
Yes, it is a true type in the Clang internal representation indeed, but not in
the original source though. Here we are giving the feedback about the source
code so it's nicer to keep it as close to the original as possible. But we are
doing similar "magic" for blocks, images and other places, so it's not that
critical at the end. Just if it can be avoided it would be better.
https://reviews.llvm.org/D35082
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
