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:
> 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.
No. For OpenCL, the condition is on line 11847 and 11848. An array type in
other address spaces is allowed.
================
Comment at: lib/Sema/SemaType.cpp:6969
+ if (state.getSema().getLangOpts().OpenCL &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
----------------
Anastasia wrote:
> 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.
Clang already deduce global and generic address spaces and use them in the
diagnostic messages. I don't see why we can use deduced global and generic
address space in diagnostics whereas cannot use deduced private address space
in diagnostics. Why users can accept deduced global and generic address spaces
but cannot accept deduced private address space?
Automatic variables and function parameters have private address space. This is
the reality and as true as a global variable has global or constant address
spaces. Not using private address space in diagnostics gives user illusion that
automatic variables and function parameters do not have address space, which is
not true.
Besides, allowing default address space to represent private address space in
AST causes ambiguity in AST. Instead of just check if a type has private
address space, now we need to check if a type has private or default address
spaces. Also if an expression has default address space, it is not clear if it
is an l-value or r-value. This will complicate semantic checking unnecessarily.
Also I am not sure if it is acceptable to modify AST between Sema and CodeGen
since it seems to change the paradigm of how clang does Sema/CodeGen now.
https://reviews.llvm.org/D35082
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
