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

[Yaxun Liu via Phabricator via cfe-commits]

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


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


================
Comment at: lib/CodeGen/CGCall.cpp:3851
+                                         ->getType()
+                                         ->getPointerAddressSpace();
         const unsigned ArgAddrSpace =
----------------
rjmccall wrote:
> 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;
>     }
>   }
It is possible RVAddrSpace is not default addr space. e.g. in OpenCL

```
global struct S g_s;

void f(struct S s);

void g() {
  f(g_s);
}

```

One thing that confuses me is that why creating temp and copying can be skipped if RVAddrSpace equals alloca addr space. The callee is supposed to work on a copy of the arg, not the arg itself, right? Shouldn't the arg always be coped to a temp then pass to the callee?


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


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


https://reviews.llvm.org/D34367