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

Yaxun Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 24 12:51:36 PDT 2017

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() {


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.


More information about the cfe-commits mailing list