[PATCH] D33842: [AMDGPU] Fix address space of global variable

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 12 inline comments as done.
yaxunl added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:969
+  /// LangAS::Default.
+  virtual unsigned getGlobalAddressSpace() const { return LangAS::Default; }
+
----------------
rjmccall wrote:
> I'm not sure this really needs to exist.
Removed.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1144
+                 ? CGM.getNSConcreteGlobalBlock()
+                 : CGM.getNullPointer(CGM.VoidPtrPtrTy,
+                                      QualType(CGM.getContext().VoidPtrTy)));
----------------
rjmccall wrote:
> You used VoidPtrTy above.
They are different fields with different types.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2549
+           AddrSpace == LangAS::opencl_local ||
+           AddrSpace >= LangAS::FirstTargetAddressSpace);
+    return AddrSpace;
----------------
rjmccall wrote:
> This does not seem like a very useful assertion.
This checks global variable has valid address space. If a global variable having private or generic address space is passed to backend, it may cause issues difficult to debug. This assertion helps to detect the issue early.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2554
+  if (K == LanguageExpected)
+    return D ? D->getType().getAddressSpace() : LangAS::Default;
+
----------------
rjmccall wrote:
> I would expect this to be the general rule for all language modes.  Why is OpenCL an exception?
> 
> It looks to me like GetGlobalVarAddressSpace is only ever called with a possibly-null D by GetOrCreateLLVMGlobal, which is only called with a null D by CreateRuntimeVariable.  I expect that every call to CreateRuntimeVariable will probably need to be audited if it's going to suddenly start returning pointers that aren't in the default address space, and we'll probably end up needing some very different behavior at that point.  So for now, let's just move the LanguageExpected case out to the one call site that uses it.
> 
> I do like the simplification of just taking the declaration instead of taking that and an address space.
For OpenCL, TargetFavored and LanguageExpected are the same, i.e. global. For C++, TargetFavored is globa, but LanguageExpected is Default.

Done.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2578
+  }
+  return getTarget().getGlobalAddressSpace();
 }
----------------
rjmccall wrote:
> Okay, I think I see what you're trying to do here: when you compile normal (i.e. non-OpenCL) code for your target, you want to implicitly change where global variables are allocated by default, then translate the pointer into LangAS::Default.  It's essentially the global-variable equivalent of what we're already doing with stack pointers.
> 
> The right way to do this is to make a hook on the TargetCodeGenInfo like getASTAllocaAddressSpace() which allows the target to override the address space for a global on a case-by-case basis.  You can then do whatever target-specific logic you need there, and you won't need to introduce getGlobalAddressSpace() on TargetInfo.  The default implementation, of course, will just return D->getType().getAddressSpace().
Done.

I am wondering if I should just move the whole function to TargetCodeGenInfo::getGlobalVarAddressSpace, which seems cleaner.


================
Comment at: lib/CodeGen/TargetInfo.cpp:7418
+  if (M.getLangOpts().OpenCL)
+    appendOpenCLVersionMD(M);
 }
----------------
rjmccall wrote:
> You have a couple things in this patch that just touch your target and are basically unrelated to the rest of your logic.  Please split those into a separate patch.
Done.


https://reviews.llvm.org/D33842