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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
