[PATCH] D34367: CodeGen: Fix address space of indirect function argument

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGCall.cpp:3832
+                                     "indirect-arg-temp");
+        IRCallArgs[FirstIRArg] = CastToAllocaAddrSpace(Addr.getPointer());
 
----------------
Isn't the original code here correct?  You're basically just adding unnecessary casts.


================
Comment at: lib/CodeGen/CGCall.cpp:3851
+                                         ->getType()
+                                         ->getPointerAddressSpace();
         const unsigned ArgAddrSpace =
----------------
Hmm.  Is there actually a test case where Addr.getType()->getAddressSpace() is not the lowering of LangAS::Default?  The correctness of the performAddrSpaceCast call you make depends on that.

If there isn't, I think it would be better to add a target hook to attempt to peephole an addrspace cast:
  llvm::Value *tryPeepholeAddrSpaceCast(llvm::Value*, unsigned valueAS, unsigned targetAS);

And then you can just do
  bool HasAddrSpaceMismatch = CGM.getASTAllocaAddrSpace() != LangAS::Default);
  if (HasAddrSpaceMismatch) {
    if (llvm::Value *ptr = tryPeepholeAddrSpaceCast(Addr.getPointer(), LangAS::Default, getASTAllocAddrSpace())) {
      Addr = Address(ptr, Addr.getAlignment()); // might need to cast the element type for this
      HasAddrSpaceMismatch = false;
    }
  }


================
Comment at: lib/CodeGen/CGCall.cpp:3861
                < Align.getQuantity()) ||
             (ArgInfo.getIndirectByVal() && (RVAddrSpace != ArgAddrSpace))) {
           // Create an aligned temporary, and copy to it.
----------------
This should be comparing AST address spaces.


================
Comment at: lib/CodeGen/CGCall.cpp:3865
+                                     "byval-temp");
+          IRCallArgs[FirstIRArg] = CastToAllocaAddrSpace(AI.getPointer());
           EmitAggregateCopy(AI, Addr, I->Ty, RV.isVolatileQualified());
----------------
Same thing, no reason to do the casts here.


================
Comment at: lib/CodeGen/CGDecl.cpp:1828
+    auto DestAS = getContext().getTargetAddressSpace(LangAS::Default);
+    if (SrcAS != DestAS) {
+      assert(SrcAS == CGM.getDataLayout().getAllocaAddrSpace());
----------------
This should be comparing AST address spaces.


https://reviews.llvm.org/D34367