Anastasia added inline comments.
================ Comment at: include/clang/AST/ASTContext.h:2328 + return AddrSpaceMapMangling || + AS >= LangAS::target_first; } ---------------- So we couldn't use the LangAS::Count instead? I have the same comment in other places that use LangAS::target_first. Why couldn't we simply use LangAS::Count? It there any point in having two tags? Another comment is why do we need ASes specified by `__attribute__((address_space(n)))` to be unique enum number at the end of named ASes of OpenCL and CUDA? I think conceptually the full range of ASes can be used in C because the ASes from OpenCL and CUDA are not available there anyways. ================ Comment at: include/clang/AST/Type.h:339-340 + auto Addr = getAddressSpace(); + if (Addr == 0) + return 0; + return Addr - LangAS::target_first; ---------------- t-tye wrote: > Since you mention this is only used for `__attribute__((address_space(n)))`, > why is this check for 0 needed? > > If it is needed then to match other places should it simply be: > > ``` > if (Addr) > return Addr - LangAS::target_first; > return 0; > ``` Could we use LangAS::Count instead? ================ Comment at: lib/AST/ASTContext.cpp:8732 + Type = Context.getAddrSpaceQualType(Type, AddrSpace + + LangAS::target_first); Str = End; ---------------- Also here, could we use LangAS::Count instead? ================ Comment at: lib/AST/ASTContext.cpp:9556 + if (AS == LangAS::Default && LangOpts.OpenCL) + return getTargetInfo().getDataLayout().getAllocaAddrSpace(); + if (AS >= LangAS::target_first) ---------------- t-tye wrote: > An alternative to doing this would be to add an opencl_private to LangAS and > have each target map it accordingly. Then this could be: > > ``` > // If a target specific address space was specified, simply return it. > if (AS >= LangAS::target_first) > return AS - LangAS::target_first; > // For OpenCL, only function local variables are not explicitly marked with > // an address space in the AST, so treat them as the OpenCL private address > space. > if (!AS && LangOpts.OpenCL) > AS = LangAS::opencl_private; > return (*AddrSpaceMap)[AS]; > ``` > This seems to better express what is happening here. If no address space was > specified, and the language is OpenCL, then treat it as OpenCL private and > map it according to the target mapping. > > If wanted to eliminate the LangAS::Default named constant then that would be > possible as it is no longer being used by name. However, would need to ensure > that the first named enumerators starts at 1 so that 0 is left as the "no > value explicitly specified" value that each target must map to the target > specific generic address space. I would very much like to see `opencl_private` represented explicitly. This would allow us to simplify some parsing and also enable proper support of `NULL ` literal (that has no AS by the spec). We can of course do this refactoring work as a separate step. ================ Comment at: lib/Sema/SemaType.cpp:5537 + llvm::APSInt Offset(addrSpace.getBitWidth(), false); + Offset = LangAS::target_first; + addrSpace += Offset; ---------------- Do we need this temporary variable here? https://reviews.llvm.org/D31404 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits