[PATCH] D33706: [AMDGPU] Fix address space for global and temporary variables in C++

[John McCall via Phabricator via cfe-commits]

rjmccall requested changes to this revision.
rjmccall added a comment.
This revision now requires changes to proceed.

Please split this up into two patches:

- one for your target-specific changes to the global address space and so on
- one for the generic fix to CreateTempAlloca etc.



================
Comment at: lib/CodeGen/CGExpr.cpp:76
+    V = getTargetHooks().performAddrSpaceCast(
+        *this, V, getASTAllocaAddressSpace(), DestAddrSpace,
+        Ty->getPointerTo(DestAddrSpace), /*non-null*/ true);
----------------
These arguments should both be AST address spaces, not lowered address spaces.  That is, this second argument should be LangAS::Default, not DestAddrSpace.

...we should really make proper types for these instead of always passing them around as unsigneds.


================
Comment at: lib/CodeGen/CodeGenFunction.h:1931
+  /// alloca will be casted to the address space expected by the language,
+  /// otherwise it stays in the alloca address space.
+  llvm::AllocaInst *CreateTempAlloca(llvm::Type *Ty, const Twine &Name = "tmp",
----------------
"On certain targets, the alloca address space is not the same as the generic address space.
Since the address of a temporary is often exposed to the program as a pointer or reference
in the generic address space, this function will convert the returned address into the generic
address space by default.  That conversion may be suppressed by passing false as
\p CastToGenericAddrSpace."

And please rename DoCast to CastToGenericAddrSpace.


================
Comment at: lib/CodeGen/CodeGenFunction.h:1973
+  /// appropriate alignment. Cast it to the expected address space if \p DoCast
+  /// is true.
+  Address CreateMemTemp(QualType T, const Twine &Name = "tmp",
----------------
"generic" instead of "expected", please, and rename DoCast to CastToGenericAddrSpace.


https://reviews.llvm.org/D33706