[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: include/clang/AST/Type.h:332
 
+  bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+  void setImplicitAddressSpaceFlag(bool Value) {
----------------
Could we add a bit of comment somewhere explaining why we added implicit AS flag please.


================
Comment at: lib/AST/Expr.cpp:3254
+        // Only (void*)0 or equivalent are treated as nullptr. If pointee type
+        // has non-default address space it is not treated as nullptr.
+        bool PointeeHasDefaultAS = false;
----------------
Is this comment right wrt to named address spaces. So is `(private void*)0` not a `nullptr`?


================
Comment at: lib/AST/Expr.cpp:3257
+        if (Ctx.getLangOpts().OpenCL)
+          PointeeHasDefaultAS =
+              (Ctx.getLangOpts().OpenCLVersion >= 200 &&
----------------
Can we now simplify this by checking implicit AS bit instead?


================
Comment at: lib/Sema/SemaType.cpp:6974
 
+  if (state.getSema().getLangOpts().OpenCL && !hasOpenCLAddressSpace &&
+      type.getAddressSpace() == LangAS::Default &&
----------------
I am not very convinced we need to do all this check really... I think since we have to live with this code further and maintain it let's try to simplify it a bit. This can also help us avoid redundant checks. Perhaps we could even move it out in a separate function?

How about some sort of hierarchical check structure like:

| |**CL version?** ||
|<2.0||>=2.0|
|`private`||**Type?**||
||pointer||non-pointer|
||`generic`||**Scope?**||
|||program||function|
|||`global`||**Qualifiers?**|
||||none||static/extern|
||||`private`||`global`|



================
Comment at: lib/Sema/SemaType.cpp:6994
+      // OpenCL v1.2 s6.5:
+      // The generic address space name for arguments to a function in a
+      // program, or local variables of a function is __private. All function
----------------
The generic address space -> The default address space


https://reviews.llvm.org/D35082