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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits