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

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 20 09:19:09 PDT 2017


rjmccall added inline comments.


================
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 {
----------------
"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."


================
Comment at: lib/CodeGen/CGExpr.cpp:357
+        unsigned AddrSpace =
+            CGF.getTarget().getTargetConstantAddressSpace().getValueOr(0);
         auto *GV = new llvm::GlobalVariable(
----------------
This isn't the right logic.  Part of what I was saying before is that this optimization isn't obviously supportable on all targets.  You should call getTargetConstantAddressSpace() and then only try to do this optimization if it doesn't return None.

For parity, the default implementation should return 0.


================
Comment at: lib/CodeGen/CGExpr.cpp:449
+                         Var, ConvertTypeForMem(E->getType())->getPointerTo()),
                      Object.getAlignment());
     // If the temporary is a global and has a constant initializer or is a
----------------
Every time you're tempted to use getPointerCast, you should instead be using the target hook to lift the pointer into the right address space.


https://reviews.llvm.org/D33842





More information about the cfe-commits mailing list