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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


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


================
Comment at: include/clang/Basic/TargetInfo.h:959
+  /// \brief Return the target address space which is read only and can be
+  /// casted to the generic address space.
+  virtual llvm::Optional<unsigned> getTargetConstantAddressSpace() const {
----------------
yaxunl wrote:
> rjmccall wrote:
> > "Return an AST address space which can be used opportunistically for constant global memory.  It must be possible to convert pointers into this address space to LangAS::Default.  If no such address space exists, this may return None, and such optimizations will be disabled."
> I will change it. Also I think getConstantAddressSpace may be a better name since we will return AST addr space.
Sorry, I typo'ed my original suggestion.  "pointers in this address space".


================
Comment at: lib/CodeGen/CGBlocks.cpp:1144
+                 ? CGM.getNSConcreteGlobalBlock()
+                 : CGM.getNullPointer(CGM.VoidPtrPtrTy,
+                                      QualType(CGM.getContext().VoidPtrTy)));
----------------
You used VoidPtrTy above.


================
Comment at: lib/CodeGen/CGExpr.cpp:342
+static Address createReferenceTemporary(CodeGenFunction &CGF,
+                                        const TargetCodeGenInfo &TCG,
+                                        const MaterializeTemporaryExpr *M,
----------------
Why are you passing the TargetCodeGenInfo down separately when it can be easily reacquired from the CodeGenFunction?

Oh, it looks like getTargetHooks() is private; there's no reason for that, just make it public.


================
Comment at: lib/CodeGen/CGExpr.cpp:372
+                    ->getPointerTo(CGF.getContext().getTargetAddressSpace(
+                        LangAS::Default)));
+          // FIXME: Should we put the new global into a COMDAT?
----------------
GV->getValueType()->getPointerTo(...)


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2549
+           AddrSpace == LangAS::opencl_local ||
+           AddrSpace >= LangAS::FirstTargetAddressSpace);
+    return AddrSpace;
----------------
This does not seem like a very useful assertion.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2554
+  if (K == LanguageExpected)
+    return D ? D->getType().getAddressSpace() : LangAS::Default;
+
----------------
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.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2571
+         AddrSpace >= LangAS::FirstTargetAddressSpace);
+  if (K == LanguageExpected || AddrSpace != LangAS::Default)
+    return AddrSpace;
----------------
It's impossible for K to be LanguageExpected here, you already checked it above.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2578
+  }
+  return getTarget().getGlobalAddressSpace();
 }
----------------
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().


================
Comment at: lib/CodeGen/TargetInfo.cpp:7418
+  if (M.getLangOpts().OpenCL)
+    appendOpenCLVersionMD(M);
 }
----------------
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.


https://reviews.llvm.org/D33842