t-tye added inline comments.
================
Comment at: include/clang/AST/Type.h:339-340
+ auto Addr = getAddressSpace();
+ if (Addr == 0)
+ return 0;
+ return Addr - LangAS::target_first;
----------------
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;
```
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2451-2453
+def warn_attribute_address_space_negative : Warning<
+ "address space is negative">,
+ InGroup<DiagGroup<"addr-space">>;
----------------
Now the one questionable test has been fixed, should the handling of
address_space attribute go back to it being an error if n is negative? That
seems more logical.
================
Comment at: lib/AST/ASTContext.cpp:9555
+ // alloca.
+ if (AS == LangAS::Default && LangOpts.OpenCL)
+ return getTargetInfo().getDataLayout().getAllocaAddrSpace();
----------------
To be consistent with other places should this simply be:
```
if (!AS && LangOpts.OpenCL)
```
================
Comment at: lib/AST/ASTContext.cpp:9556
+ if (AS == LangAS::Default && LangOpts.OpenCL)
+ return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+ if (AS >= LangAS::target_first)
----------------
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.
================
Comment at: lib/Sema/SemaExprCXX.cpp:2052
+ else if (AllocType.getQualifiers().getAddressSpace() &&
+ AllocType.getQualifiers().getAddressSpace() !=
LangAS::target_first)
return Diag(Loc, diag::err_address_space_qualified_new)
----------------
Should this be `>= LangAS::target_first` ? Seems it is wanting to check if an
`__attribute__((address_space(n)))` was specified.
================
Comment at: lib/Sema/SemaExprCXX.cpp:3123
+ if (Pointee.getQualifiers().getAddressSpace() &&
+ Pointee.getQualifiers().getAddressSpace() != LangAS::target_first)
return Diag(Ex.get()->getLocStart(),
----------------
Ditto.
https://reviews.llvm.org/D31404
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
