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: > > 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. > > 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? > > Yes, we did this initially as a workaround because there was no way to > distinguish the private and default address space. So now since we are adding > the private AS explicitly, it would be nice to remove the workaround if > that's possible. It's just a messy code which is hard to understand and we > had places in which we need to know if the address space was specified > explicitly or now. NULL pointer is one of them. There were also bugs > associated to this because it wasn't expected that the address space is > 'magically' attached during parsing. Even though most of the things are > solved now... I would prefer not to deduce any attributes at this place not > just private but global and default as well... > > > 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. > > I don't see any ambiguity, this is a difference in OpenCL wrt C that we have > implicit address space which we have to handle in the compiler too. The > question is what's the best place to loose the information about default > address space... so we first went for removing it early in parsing but there > were a number of issues for example NULL couldn't be handled properly and > some other bugs. The original source couldn't be printed back the same way. > I personally find the current solution a bit hacky because this function was > just supposed to simply parse the attribute and not to add bits that don't > exist in the original source code. Therefore, the code is overcomplicated > because we are trying to get some extra information about Declarator > properties which this function shouldn't really touch. > > Once again I have never said it is wrong to add private address space to > those objects, just it will be cleaner in my view if we don't do it at this > stage. If it's too difficult to do it differently (e.g too big of the change) > I am OK to accept this solution. But if it's possible to do it later, I would > prefer to keep default address space with no address space attribute until > the last point where we generate the IR. > > > 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. > > Could you please elaborate what is the difficulty here? Address space is a type qualifier applies to all expressions, not just variables. Using default address space in AST means that the real address space of any expression will depend on the context and needs to be evaluated whenever it is needed for semantic checking, which may result in lots of changes in Sema. Since most semantic checking uses the real address space, it is more efficient to use real address space in AST. The only semantic checking that requires to know the original default address space seems to be (void*)0 for OpenCL 1.2 since by OpenCL spec it should be a private pointer but by convention it is treated as null pointer. It does not make sense to use default address space in AST just for this one special case. I think we should be able to handle it through some other approach. John, do you have any comments? 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
