CodeGen: Fix address space of indirect function argument

[yaxunl]

Summary

The indirect function argument is in alloca address space in LLVM IR. However,
during Clang codegen for C++, the address space of indirect function argument
should match its address space in the source code, i.e., default addr space, even
for indirect argument. This is because destructor of the indirect argument may
be called in the caller function, and address of the indirect argument may be
taken, in either case the indirect function argument is expected to be in default
addr space, not the alloca address space.

Therefore, the indirect function argument should be mapped to the temp var
casted to default address space. The caller will cast it to alloca addr space
when passing it to the callee. In the callee, the argument is also casted to the
default address space and used.

CallArg is refactored to facilitate this fix.

[rjmccall]

lib/CodeGen/CGCall.cpp

1605

Everything about your reasoning seems to apply to normal indirect arguments, too.

[yaxunl]

lib/CodeGen/CGCall.cpp

1605

non-byval indirect argument is normally in default address space instead of alloca address space. For example,

struct A { int a;};
void f(A &x);

x is an indirect argument but should not be in alloca addr space, since the caller may pass a reference to a global variable.

[rjmccall]

lib/CodeGen/CGCall.cpp

1605

'x' is not an indirect argument in this context. It is a direct argument that happens to be of reference type.

A normal Indirect argument is when the ABI says an argument should implicitly be passed as a pointer to a temporary. IndirectByVal is when the ABI says that something should be passed directly in the arguments area of the stack. Some targets never use indirect arguments for normal C cases, but they're still used for direct non-POD arguments in C++.

[yaxunl]

lib/CodeGen/CGCall.cpp

1605

You are right. Fixed.

[yaxunl]

Always put indirect arg in alloca addr space.

[rjmccall]

lib/CodeGen/CGCall.cpp

3832

Isn't the original code here correct? You're basically just adding unnecessary casts.

3851

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;
  }
}

3861

This should be comparing AST address spaces.

3865

Same thing, no reason to do the casts here.

lib/CodeGen/CGDecl.cpp

1828

This should be comparing AST address spaces.

[yaxunl]

lib/CodeGen/CGCall.cpp

3832

will remove.

3851

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?

3865

will remove

lib/CodeGen/CGDecl.cpp

1828

will do.

[rjmccall]

lib/CodeGen/CGCall.cpp

3851

The result of emitting an agg expression should already be a temporary. All sorts of things would be broken if it we still had a direct reference to g_s when we got here.

[yaxunl]

lib/CodeGen/CGCall.cpp

3851

For the above example, the AST for the call statement f(g_s) is

`-CallExpr 0x60a9fc0 <line:10:3, col:8> 'void'
  |-ImplicitCastExpr 0x60a9fa8 <col:3> 'void (*)(struct S)' <FunctionToPointerDecay>
  | `-DeclRefExpr 0x60a9f00 <col:3> 'void (struct S)' Function 0x60a9d80 'f' 'void (struct S)'
  `-ImplicitCastExpr 0x60a9ff0 <col:5> 'struct S':'struct S' <LValueToRValue>
    `-DeclRefExpr 0x60a9f28 <col:5> '__global struct S':'__global struct S' lvalue Var 0x607efb0 'g_s' '__global struct S':'__global struct S'

When clang executes line 3846 of CGCall.cpp, RV contains

@g_s = external addrspace(1) global %struct.S, align 4

Therefore the temporary var has not been created yet. It seems the temporary var will be created by line 3864.

So at line 3848, RVAddrSpace has non-default address space 1.

[yaxunl]

lib/CodeGen/CGCall.cpp

3861

The AST address space of RV cannot be obtained through CGFunctionInfo::const_arg_iterator it and it->type since it->type takes type of

ImplicitCastExpr 0x60a9ff0 <col:5> 'struct S':'struct S' <LValueToRValue>
    `-DeclRefExpr 0x60a9f28 <col:5> '__global struct S':'__global struct S' lvalue Var 0x607efb0

and the original addr space is lost due to LValueToRValue cast.

To get the AST addr space of RV, it seems I need to save the argument Expr in CallArgList and get it from Expr.

[rjmccall]

lib/CodeGen/CGCall.cpp

3861

I think your last two comments are related. I'm not sure why we haven't copied into a temporary here, and if we had, the assumption of LangAS::Default would be fine. Would you mind doing the investigation there?

[yaxunl]

lib/CodeGen/CGCall.cpp

3861

It seems the backend will insert a temp copy for byval arguments, therefore normally a byval argument does not need caller to create a temp copy in LLVM IR. An explicit temp copy is only needed for special cases, e.g. alignment mismatch with ABI.

For example, the following C program,

struct S {
  long x[100];
};

struct S g_s;

void f(struct S s);

void g() {
  f(g_s);
}

will generate the following IR on x86_64:

target triple = "x86_64-unknown-linux-gnu"

%struct.S = type { [100 x i64] }

@g_s = common global %struct.S zeroinitializer, align 8

; Function Attrs: noinline nounwind optnone
define void @g() #0 {
entry:
  call void @f(%struct.S* byval align 8 @g_s)
  ret void
}

declare void @f(%struct.S* byval align 8) #1

However, the following C program

struct S {
  int x[100];
};

struct S g_s;

void f(struct S s);

void g() {
  f(g_s);
}

will generate the following IR

target triple = "x86_64-unknown-linux-gnu"

%struct.S = type { [100 x i32] }

@g_s = common global %struct.S zeroinitializer, align 4

; Function Attrs: noinline nounwind optnone
define void @g() #0 {
entry:
  %byval-temp = alloca %struct.S, align 8
  %0 = bitcast %struct.S* %byval-temp to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %0, i8* bitcast (%struct.S* @g_s to i8*), i64 400, i32 4, i1 false)
  call void @f(%struct.S* byval align 8 %byval-temp)
  ret void
}

declare void @f(%struct.S* byval align 8) #1

The temp var is generated by line 3863. The control flow reaches line 3863 because the alignment of the argument is 4 but the ABI requires it to be 8, so a temp is created to match the ABI align requirement.

That means, in the OpenCL example, it is normal that a temp var is not generated at line 3848. The temp is supposed to be generated at line 3863 too, like the C example.

[yaxunl]

lib/CodeGen/CGCall.cpp

3861

For C++, it is different story, the AST is

`-FunctionDecl 0x64abee0 <line:9:1, line:11:1> line:9:6 g 'void (void)'
  `-CompoundStmt 0x64ac388 <col:10, line:11:1>
    `-CallExpr 0x64ac060 <line:10:3, col:8> 'void'
      |-ImplicitCastExpr 0x64ac048 <col:3> 'void (*)(struct S)' <FunctionToPointerDecay>
      | `-DeclRefExpr 0x64abff8 <col:3> 'void (struct S)' lvalue Function 0x64abe20 'f' 'void (struct S)'
      `-CXXConstructExpr 0x64ac278 <col:5> 'struct S' 'void (const struct S &) throw()'
        `-ImplicitCastExpr 0x64ac090 <col:5> 'const struct S' lvalue <NoOp>
          `-DeclRefExpr 0x64abfd0 <col:5> 'struct S' lvalue Var 0x64ab938 'g_s' 'struct S'

The argument is a copy constructor expr, therefore a temporary copy is always created, which is in alloca addr space.

For C and OpenCL, since there is no copy constructor, the temporary copy is not created.

[yaxunl]

Revised by John's comments.

[rjmccall]

lib/CodeGen/CGCall.h

1

Ah, I think I understand what's going on here now. "indirect byval" arguments include an implicit copy to the stack, and the code is trying to avoid emitting an extra copy to a temporary in the frontend. It does this by recognizing loads from aggregate l-values and just adding the l-value to the call-argument list as a "needs copy" argument. Call-argument preparation then recognizes that the argument is being passed indirect byval and, under certain conditions, just uses the l-value address as the IR argument.

Instead of creeping more and more information from the LValue into the CallArg, I think there are two reasonable alternatives:

• Suppress this optimization in EmitCallArg when the LValue isn't in the alloca address space. EmitCallArg already suppresses it when the argument is under-aligned.

• Find a way to actually store an LValue in a CallArg. This has the advantage that we can assert if someone tries to access it as an RValue, which is good because any attempt to use a needs-copy argument as if it were a normal argument is a lurking bug.

[yaxunl]

Revised by John's comments. Removed address space from CallArg and added l-value expression instead.

[yaxunl]

lib/CodeGen/CGCall.h

1

Sorry for the delay. I updated the patch with by the second approach. I did not add a member of LValue type to CallArg but added a member of Expr* type since sometimes LValue is not available. The Expr is the l-value expression from which the r-value is derived.

[rjmccall]

That's not really okay; there are some places that make guarantees about call-argument ordering, and in general we want that to be decided at a higher level, rather than having a particular order forced in corner cases just to satisfy a lower-level implementation requirement.

[yaxunl]

In D34367#1009021, @rjmccall wrote:

That's not really okay; there are some places that make guarantees about call-argument ordering, and in general we want that to be decided at a higher level, rather than having a particular order forced in corner cases just to satisfy a lower-level implementation requirement.

I see your point. Actually we only need to let CallArg to convey the information that some indirect arg needs a temporary copy. There is already a member NeedsCopy in CallArg. I will see if I can just re-use that.

[yaxunl]

Extends NeedsCopy of CallArg for indirect byval arguments.

[rjmccall]

lib/CodeGen/CGCall.cpp

3505

This is an odd condition. What are you really trying to express here? Something about the IR address space that the AST address space maps to?

The condition AS != LangAS::Default kills the optimization for all practical purposes, by the way, so something does need to change here.

I will note that if CallArg could take an LValue, neither this nor the alignment check would be required here; they'd just be done by the call-emission code.

3511

Please just add an operator| for CallArg::Flag.

lib/CodeGen/CGCall.h

216

I thing Flags would be a better name.

[yaxunl]

lib/CodeGen/CGCall.cpp

3505

This checks if an indirect by val arg can omit the copying. If the arg is in alloca or default addr space, the backend is able to copy it, so no explicit copying is needed by clang. Otherwise we need clang to insert copying.

I will try to replace Flag with LVal and see if it simplifies things.

[yaxunl]

Replace Flag with LValue in CallArg.

[rjmccall]

lib/CodeGen/CGCall.cpp

3510

Please move all this conditionality to the call-emission code; just check whether you have a load of an aggregate l-value and, if so, add the LValue as an unloaded aggregate to the argument list.

lib/CodeGen/CGCall.h

221

It should be either an RValue or an LValue, not both. The client needs to do different things in the different cases. But you should add a convenience method for getting the argument as an RValue that either returns the stored RValue or copies out of the stored LValue, and most of the cases can just use that.

Just overload the constructor and CallArgList::add. I would call the latter addUncopiedAggregate.

[yaxunl]

Revised by John's comments.

[yaxunl]

sync to ToT.

[rjmccall]

lib/CodeGen/CGCall.cpp

3446

No, I don't think this is right. This method should always return an independent RValue; if the CallArg is storing an LValue, it should copy from it into a temporary. That means it needs to take a CGF, and it also means you can't call it eagerly.

3779

For example, all of this is eagerly forcing an RValue, and it needs to be delayed so that you can take advantage of having an LValue in the right cases below.

3814

I think most of this could be replaced with a copyInto(CGF&, Address) method on CallArg that just stores/copies the RValue into the destination. But you might need to handle the aggregate-RValue case specially before calling the method, because I think what that's expressing is that we're trying to evaluate aggregate-RValue arguments directly into the right place in the inalloca allocation.

[yaxunl]

Revised by John's comments.

Added CallArg::copyInto and modified CallArg::getRValue() to return an independent r-value by default. However some cases expecting l-value not copied, therefore I added an optional argument to CallArg::getRValue() to return l-value as aggregate directly.

[yaxunl]

Clean up CallArg::getRValue() so that it always return independent value.

[rjmccall]

I'm sorry, I did a review of your patch on Phabricator but apparently never submitted it.

lib/CodeGen/CGCall.h

219

This could be clearer. I would say something like "The argument is semantically a load from this l-value."

220

Why the two different naming conventions for these parameters? Just call the new one lv for consistency, please.

221

Please add an IsUsed flag so that you can assert that e.g. getRValue and/or copyInto aren't called twice. It's okay for this to be mutable, since it's just a data-flow assertion rather than logically a change to the value of the argument.

234

Please name this something like getKnownLValue and add a parallel getKnownRValue. These should assert !IsUsed but not set it.

248

Part of my thinking in suggesting this representation change was that the current representation was prone to a certain kind of bug where clients blindly use the RValue without realizing that it's actually something they need to copy from instead of using directly. That is generally a bug because indirect arguments are expected to be independent slots and are not permitted to alias. The new representation implicitly fixes these bugs by pushing users towards using one of these approaches.

All of this is to say that I'm not thrilled about having a getAggregateAddress here.

[yaxunl]

lib/CodeGen/CGCall.h

248

I see. I will remove getAggregateAddress.

[yaxunl]

Revised by John's comments. Removed CallArg::getAggregateAddress().

[rjmccall]

lib/CodeGen/CGCall.cpp

3427

I think it's okay to do this because of a reasonable assumption that pointer arguments are never emitted as LValues, but you should leave that as a comment.

[yaxunl]

Added comment about emit non-null argument check.

[rjmccall]

Thanks. LGTM!

lib/CodeGen/CGCall.h

 //===----- CGCall.h - Encapsulate calling convention details ----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
public:
     }
 
     /// If this is a delayed callee computation of some sort, prepare
     /// a concrete callee.
     CGCallee prepareConcreteCallee(CodeGenFunction &CGF) const;
   };
 
   struct CallArg {
     RValue RV;
     QualType Ty;
     bool NeedsCopy;
-    CallArg(RValue rv, QualType ty, bool needscopy)
-    : RV(rv), Ty(ty), NeedsCopy(needscopy)
-    { }
+    const Expr *LV; // The l-value expression from which RV is derived
+    CallArg(RValue rv, QualType ty, bool needscopy, const Expr *LVal = nullptr)
+        : RV(rv), Ty(ty), NeedsCopy(needscopy), LV(LVal) {}
   };
 
   /// CallArgList - Type for representing both the value and type of
   /// arguments in a call.
   class CallArgList :
     public SmallVector<CallArg, 16> {
   public:
     CallArgList() : StackBase(nullptr) {}
 
     struct Writeback {
       /// The original argument.  Note that the argument l-value
       /// is potentially null.
       LValue Source;
 
       /// The temporary alloca.
       Address Temporary;
 
       /// A value to "use" after the writeback, or null.
       llvm::Value *ToUse;
     };
 
     struct CallArgCleanup {
       EHScopeStack::stable_iterator Cleanup;
 
       /// The "is active" insertion point.  This instruction is temporary and
       /// will be removed after insertion.
       llvm::Instruction *IsActiveIP;
     };
 
-    void add(RValue rvalue, QualType type, bool needscopy = false) {
-      push_back(CallArg(rvalue, type, needscopy));
+    void add(RValue rvalue, QualType type, bool needscopy = false,
+             const Expr *LVal = nullptr) {
+      push_back(CallArg(rvalue, type, needscopy, LVal));
     }
 
     /// Add all the arguments from another CallArgList to this one. After doing
     /// this, the old CallArgList retains its list of arguments, but must not
     /// be used to emit a call.
     void addFrom(const CallArgList &other) {
       insert(end(), other.begin(), other.end());
       Writebacks.insert(Writebacks.end(),

lib/CodeGen/CGCall.cpp

   void Emit(CodeGenFunction &CGF, Flags flags) override {
     const CXXDestructorDecl *Dtor = Ty->getAsCXXRecordDecl()->getDestructor();
     assert(!Dtor->isTrivial());
     CGF.EmitCXXDestructorCall(Dtor, Dtor_Complete, /*for vbase*/ false,
                               /*Delegating=*/false, Addr);
   }
 };
 
 struct DisableDebugLocationUpdates {
   CodeGenFunction &CGF;
   bool disabledDebugInfo;
   DisableDebugLocationUpdates(CodeGenFunction &CGF, const Expr *E) : CGF(CGF) {
     if ((disabledDebugInfo = isa<CXXDefaultArgExpr>(E) && CGF.getDebugInfo()))
       CGF.disableDebugInfo();
   }
   ~DisableDebugLocationUpdates() {
     if (disabledDebugInfo)
       CGF.enableDebugInfo();
   }
 };
 
 } // end anonymous namespace
 
 void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
                                   QualType type) {
   DisableDebugLocationUpdates Dis(*this, E);
+
   if (const ObjCIndirectCopyRestoreExpr *CRE
         = dyn_cast<ObjCIndirectCopyRestoreExpr>(E)) {
     assert(getLangOpts().ObjCAutoRefCount);
     return emitWritebackArg(*this, args, CRE);
   }
 
   assert(type->isReferenceType() == E->isGLValue() &&
          "reference binding to unmaterialized r-value!");
 
   if (E->isGLValue()) {
     assert(E->getObjectKind() == OK_Ordinary);
-    return args.add(EmitReferenceBindingToExpr(E), type);
+    return args.add(EmitReferenceBindingToExpr(E), type, false, E);
   }
 
   bool HasAggregateEvalKind = hasAggregateEvaluationKind(type);
 
   // In the Microsoft C++ ABI, aggregate arguments are destructed by the callee.
   // However, we still have to push an EH-only cleanup in case we unwind before
   // we make it to the call.
   if (HasAggregateEvalKind && getContext().isParamDestroyedInCallee(type)) {

         RD && RD->hasNonTrivialDestructor() &&
         (CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default ||
          RD->hasTrivialABIOverride());
     if (DestroyedInCallee)
       Slot.setExternallyDestructed();
 
     EmitAggExpr(E, Slot);
     RValue RV = Slot.asRValue();
-    args.add(RV, type);
+    args.add(RV, type, false, E);
 
     if (DestroyedInCallee) {
       // Create a no-op GEP between the placeholder and the cleanup so we can
       // RAUW it successfully.  It also serves as a marker of the first
       // instruction where the cleanup is active.
       pushFullExprCleanup<DestroyUnpassedArg>(EHCleanup, Slot.getAddress(),
                                               type);
       // This unreachable is a temporary marker which will be removed later.
       llvm::Instruction *IsActive = Builder.CreateUnreachable();
       args.addArgCleanupDeactivation(EHStack.getInnermostEHScope(), IsActive);
     }
     return;
   }
 
   if (HasAggregateEvalKind && isa<ImplicitCastExpr>(E) &&
       cast<CastExpr>(E)->getCastKind() == CK_LValueToRValue) {
     LValue L = EmitLValue(cast<CastExpr>(E)->getSubExpr());
     assert(L.isSimple());
     if (L.getAlignment() >= getContext().getTypeAlignInChars(type)) {
-      args.add(L.asAggregateRValue(), type, /*NeedsCopy*/true);
+      args.add(L.asAggregateRValue(), type, /*NeedsCopy*/ true, E);
     } else {
       // We can't represent a misaligned lvalue in the CallArgList, so copy
       // to an aligned temporary now.
       LValue Dest = MakeAddrLValue(CreateMemTemp(type), type);
       EmitAggregateCopy(Dest, L, type, L.isVolatile());
-      args.add(RValue::getAggregate(Dest.getAddress()), type);
+      args.add(RValue::getAggregate(Dest.getAddress()), type, false, E);
     }
     return;
   }
 
-  args.add(EmitAnyExprToTemp(E), type);
+  args.add(EmitAnyExprToTemp(E), type, false, E);
 }
 
 QualType CodeGenFunction::getVarArgType(const Expr *Arg) {
   // System headers on Windows define NULL to 0 instead of 0LL on Win64. MSVC
   // implicitly widens null pointer constants that are arguments to varargs
   // functions to pointer-sized ints.
   if (!getTarget().getTriple().isOSWindows())
     return Arg->getType();

     RValue RV = I->RV;
 
     // Insert a padding argument to ensure proper alignment.
     if (IRFunctionArgs.hasPaddingArg(ArgNo))
       IRCallArgs[IRFunctionArgs.getPaddingArgNo(ArgNo)] =
           llvm::UndefValue::get(ArgInfo.getPaddingType());
 
     unsigned FirstIRArg, NumIRArgs;
     std::tie(FirstIRArg, NumIRArgs) = IRFunctionArgs.getIRArgs(ArgNo);
 
     switch (ArgInfo.getKind()) {
     case ABIArgInfo::InAlloca: {
       assert(NumIRArgs == 0);
       assert(getTarget().getTriple().getArch() == llvm::Triple::x86);
       if (RV.isAggregate()) {
         // Replace the placeholder with the appropriate argument slot GEP.
         llvm::Instruction *Placeholder =

           Addr = Builder.CreateBitCast(Addr, MemType);
         LValue argLV = MakeAddrLValue(Addr, I->Ty);
         EmitInitStoreOfNonAggregate(*this, RV, argLV);
       }
       break;
     }
 
     case ABIArgInfo::Indirect: {
+      auto CastToAllocaAddrSpace = [&](llvm::Value *V) {
+        auto *T = V->getType()->getPointerElementType()->getPointerTo(
+            CGM.getDataLayout().getAllocaAddrSpace());
+        return getTargetHooks().performAddrSpaceCast(
+            *this, V, LangAS::Default, CGM.getASTAllocaAddressSpace(), T, true);
+      };
       assert(NumIRArgs == 1);
       if (RV.isScalar() || RV.isComplex()) {
         // Make a temporary alloca to pass the argument.
         Address Addr = CreateMemTemp(I->Ty, ArgInfo.getIndirectAlign(),
                                      "indirect-arg-temp", false);
         IRCallArgs[FirstIRArg] = Addr.getPointer();
 
         LValue argLV = MakeAddrLValue(Addr, I->Ty);
         EmitInitStoreOfNonAggregate(*this, RV, argLV);
       } else {
         // We want to avoid creating an unnecessary temporary+copy here;
         // however, we need one in three cases:
         // 1. If the argument is not byval, and we are required to copy the
         //    source.  (This case doesn't occur on any common architecture.)
         // 2. If the argument is byval, RV is not sufficiently aligned, and
         //    we cannot force it to be sufficiently aligned.
         // 3. If the argument is byval, but RV is located in an address space
-        //    different than that of the argument (0).
+        //    different than that of the argument (alloca address space).
         Address Addr = RV.getAggregateAddress();
         CharUnits Align = ArgInfo.getIndirectAlign();
         const llvm::DataLayout *TD = &CGM.getDataLayout();
-        const unsigned RVAddrSpace = Addr.getType()->getAddressSpace();
-        const unsigned ArgAddrSpace =
-            (FirstIRArg < IRFuncTy->getNumParams()
-                 ? IRFuncTy->getParamType(FirstIRArg)->getPointerAddressSpace()
-                 : 0);
+        LangAS RVAddrSpace = LangAS::Default;
+        if (I->LV && isa<ImplicitCastExpr>(I->LV) &&
+            cast<CastExpr>(I->LV)->getCastKind() == CK_LValueToRValue) {
+          RVAddrSpace =
+              cast<CastExpr>(I->LV)->getSubExpr()->getType().getAddressSpace();
+        }
+
+        assert((FirstIRArg >= IRFuncTy->getNumParams() ||
+             IRFuncTy->getParamType(FirstIRArg)->getPointerAddressSpace() ==
+             TD->getAllocaAddrSpace()) && "indirect argument must be in alloca address space");
         if ((!ArgInfo.getIndirectByVal() && I->NeedsCopy) ||
             (ArgInfo.getIndirectByVal() && Addr.getAlignment() < Align &&
              llvm::getOrEnforceKnownAlignment(Addr.getPointer(),
-                                              Align.getQuantity(), *TD)
-               < Align.getQuantity()) ||
-            (ArgInfo.getIndirectByVal() && (RVAddrSpace != ArgAddrSpace))) {
+                                              Align.getQuantity(),
+                                              *TD) < Align.getQuantity()) ||
+            (ArgInfo.getIndirectByVal() && RVAddrSpace != LangAS::Default &&
+             RVAddrSpace != LangAS::opencl_private &&
+             RVAddrSpace != CGM.getASTAllocaAddressSpace())) {
           // Create an aligned temporary, and copy to it.
           Address AI = CreateMemTemp(I->Ty, ArgInfo.getIndirectAlign(),
                                      "byval-temp", false);
           IRCallArgs[FirstIRArg] = AI.getPointer();
           LValue Dest = MakeAddrLValue(AI, I->Ty);
           LValue Src = MakeAddrLValue(Addr, I->Ty);
           EmitAggregateCopy(Dest, Src, I->Ty, RV.isVolatileQualified());
         } else {
           // Skip the extra memcpy call.
-          IRCallArgs[FirstIRArg] = Addr.getPointer();
+          IRCallArgs[FirstIRArg] = CastToAllocaAddrSpace(Addr.getPointer());
         }
       }
       break;
     }
 
     case ABIArgInfo::Ignore:
       assert(NumIRArgs == 0);
       break;

lib/CodeGen/CGDecl.cpp

   /// ns_consumed argument when we can't reasonably do that just by
   /// not doing the initial retain for a __block argument.
   struct ConsumeARCParameter final : EHScopeStack::Cleanup {
     ConsumeARCParameter(llvm::Value *param,
                         ARCPreciseLifetime_t precise)
       : Param(param), Precise(precise) {}
 
     llvm::Value *Param;
     ARCPreciseLifetime_t Precise;
 
     void Emit(CodeGenFunction &CGF, Flags flags) override {
       CGF.EmitARCRelease(Param, Precise);
     }
   };
 } // end anonymous namespace
 
 /// Emit an alloca (or GlobalValue depending on target)

   // If we already have a pointer to the argument, reuse the input pointer.
   if (Arg.isIndirect()) {
     DeclPtr = Arg.getIndirectAddress();
     // If we have a prettier pointer type at this point, bitcast to that.
     unsigned AS = DeclPtr.getType()->getAddressSpace();
     llvm::Type *IRTy = ConvertTypeForMem(Ty)->getPointerTo(AS);
     if (DeclPtr.getType() != IRTy)
       DeclPtr = Builder.CreateBitCast(DeclPtr, IRTy, D.getName());
+    // Indirect argument is in alloca address space, which may be different
+    // from the default address space.
+    auto AllocaAS = CGM.getASTAllocaAddressSpace();
+    auto *V = DeclPtr.getPointer();
+    auto SrcLangAS = getLangOpts().OpenCL ? LangAS::opencl_private : AllocaAS;
+    auto DestLangAS =
+        getLangOpts().OpenCL ? LangAS::opencl_private : LangAS::Default;
+    if (SrcLangAS != DestLangAS) {
+      assert(getContext().getTargetAddressSpace(SrcLangAS) ==
+             CGM.getDataLayout().getAllocaAddrSpace());
+      auto DestAS = getContext().getTargetAddressSpace(DestLangAS);
+      auto *T = V->getType()->getPointerElementType()->getPointerTo(DestAS);
+      DeclPtr = Address(getTargetHooks().performAddrSpaceCast(
+                            *this, V, SrcLangAS, DestLangAS, T, true),
+                        DeclPtr.getAlignment());
+    }
 
     // Push a destructor cleanup for this parameter if the ABI requires it.
     // Don't push a cleanup in a thunk for a method that will also emit a
     // cleanup.
     if (!IsScalar && !CurFuncIsThunk &&
         getContext().isParamDestroyedInCallee(Ty)) {
       const CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
       if (RD && RD->hasNonTrivialDestructor())

test/CodeGenCXX/amdgcn-func-arg.cpp

+// RUN: %clang_cc1 -O0 -triple amdgcn---amdgiz -emit-llvm %s -o - | FileCheck %s
+
+class A {
+public:
+  int x;
+  A():x(0) {}
+  ~A() {}
+};
+
+class B {
+int x[100];
+};
+
+A g_a;
+B g_b;
+
+void func_with_ref_arg(A &a);
+void func_with_ref_arg(B &b);
+
+// CHECK-LABEL: define void @_Z22func_with_indirect_arg1A(%class.A addrspace(5)* %a)
+// CHECK:  %p = alloca %class.A*, align 8, addrspace(5)
+// CHECK:  %[[r1:.+]] = addrspacecast %class.A* addrspace(5)* %p to %class.A**
+// CHECK:  %[[r0:.+]] = addrspacecast %class.A addrspace(5)* %a to %class.A*
+// CHECK:  store %class.A* %[[r0]], %class.A** %[[r1]], align 8
+void func_with_indirect_arg(A a) {
+  A *p = &a;
+}
+
+// CHECK-LABEL: define void @_Z22test_indirect_arg_autov()
+// CHECK:  %a = alloca %class.A, align 4, addrspace(5)
+// CHECK:  %[[r0:.+]] = addrspacecast %class.A addrspace(5)* %a to %class.A*
+// CHECK:  %agg.tmp = alloca %class.A, align 4, addrspace(5)
+// CHECK:  %[[r1:.+]] = addrspacecast %class.A addrspace(5)* %agg.tmp to %class.A*
+// CHECK:  call void @_ZN1AC1Ev(%class.A* %[[r0]])
+// CHECK:  call void @llvm.memcpy.p0i8.p0i8.i64
+// CHECK:  %[[r4:.+]] = addrspacecast %class.A* %[[r1]] to %class.A addrspace(5)*
+// CHECK:  call void @_Z22func_with_indirect_arg1A(%class.A addrspace(5)* %[[r4]])
+// CHECK:  call void @_ZN1AD1Ev(%class.A* %[[r1]])
+// CHECK:  call void @_Z17func_with_ref_argR1A(%class.A* dereferenceable(4) %[[r0]])
+// CHECK:  call void @_ZN1AD1Ev(%class.A* %[[r0]])
+void test_indirect_arg_auto() {
+  A a;
+  func_with_indirect_arg(a);
+  func_with_ref_arg(a);
+}
+
+// CHECK: define void @_Z24test_indirect_arg_globalv()
+// CHECK:  %agg.tmp = alloca %class.A, align 4, addrspace(5)
+// CHECK:  %[[r0:.+]] = addrspacecast %class.A addrspace(5)* %agg.tmp to %class.A*
+// CHECK:  call void @llvm.memcpy.p0i8.p0i8.i64
+// CHECK:  %[[r2:.+]] = addrspacecast %class.A* %[[r0]] to %class.A addrspace(5)*
+// CHECK:  call void @_Z22func_with_indirect_arg1A(%class.A addrspace(5)* %[[r2]])
+// CHECK:  call void @_ZN1AD1Ev(%class.A* %[[r0]])
+// CHECK:  call void @_Z17func_with_ref_argR1A(%class.A* dereferenceable(4) addrspacecast (%class.A addrspace(1)* @g_a to %class.A*))
+void test_indirect_arg_global() {
+  func_with_indirect_arg(g_a);
+  func_with_ref_arg(g_a);
+}
+
+// CHECK-LABEL: define void @_Z19func_with_byval_arg1B(%class.B addrspace(5)* byval align 4 %b)
+// CHECK:  %p = alloca %class.B*, align 8, addrspace(5)
+// CHECK:  %[[r1:.+]] = addrspacecast %class.B* addrspace(5)* %p to %class.B**
+// CHECK:  %[[r0:.+]] = addrspacecast %class.B addrspace(5)* %b to %class.B*
+// CHECK:  store %class.B* %[[r0]], %class.B** %[[r1]], align 8
+void func_with_byval_arg(B b) {
+  B *p = &b;
+}
+
+// CHECK-LABEL: define void @_Z19test_byval_arg_autov()
+// CHECK:  %b = alloca %class.B, align 4, addrspace(5)
+// CHECK:  %[[r0:.+]] = addrspacecast %class.B addrspace(5)* %b to %class.B*
+// CHECK:  %agg.tmp = alloca %class.B, align 4, addrspace(5)
+// CHECK:  %[[r1:.+]] = addrspacecast %class.B addrspace(5)* %agg.tmp to %class.B*
+// CHECK:  call void @llvm.memcpy.p0i8.p0i8.i64
+// CHECK:  %[[r4:.+]] = addrspacecast %class.B* %[[r1]] to %class.B addrspace(5)*
+// CHECK:  call void @_Z19func_with_byval_arg1B(%class.B addrspace(5)* byval align 4 %[[r4]])
+// CHECK:  call void @_Z17func_with_ref_argR1B(%class.B* dereferenceable(400) %[[r0]])
+void test_byval_arg_auto() {
+  B b;
+  func_with_byval_arg(b);
+  func_with_ref_arg(b);
+}
+
+// CHECK-LABEL: define void @_Z21test_byval_arg_globalv()
+// CHECK:  %agg.tmp = alloca %class.B, align 4, addrspace(5)
+// CHECK:  %[[r0:.+]] = addrspacecast %class.B addrspace(5)* %agg.tmp to %class.B*
+// CHECK:  call void @llvm.memcpy.p0i8.p0i8.i64
+// CHECK:  %[[r2:.+]] = addrspacecast %class.B* %[[r0]] to %class.B addrspace(5)*
+// CHECK:  call void @_Z19func_with_byval_arg1B(%class.B addrspace(5)* byval align 4 %[[r2]])
+// CHECK:  call void @_Z17func_with_ref_argR1B(%class.B* dereferenceable(400) addrspacecast (%class.B addrspace(1)* @g_b to %class.B*))
+void test_byval_arg_global() {
+  func_with_byval_arg(g_b);
+  func_with_ref_arg(g_b);
+}

test/CodeGenOpenCL/addr-space-struct-arg.cl

 // RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -triple amdgcn-amdhsa-amd-amdgizcl | FileCheck -enable-var-scope -check-prefixes=COM,AMD %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -triple amdgcn-amdhsa-amd | FileCheck -enable-var-scope -check-prefixes=COM,AMD %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -finclude-default-header -triple amdgcn-amdhsa-amd | FileCheck -enable-var-scope -check-prefixes=COM,AMD,AMD20 %s
 
 typedef struct {
   int cells[9];
 } Mat3X3;
 
 typedef struct {
   int cells[16];
 } Mat4X4;
struct LargeStructOneMember {
   int2 x[100];
 };
 
 struct LargeStructTwoMember {
   int2 x[40];
   int2 y[20];
 };
 
+#if __OPENCL_C_VERSION__ >= 200
+struct LargeStructOneMember g_s;
+#endif
 
 // X86-LABEL: define void @foo(%struct.Mat4X4* noalias sret %agg.result, %struct.Mat3X3* byval align 4 %in)
 // AMD-LABEL: define %struct.Mat4X4 @foo([9 x i32] %in.coerce)
 Mat4X4 __attribute__((noinline)) foo(Mat3X3 in) {
   Mat4X4 out;
   return out;
 }
 

 }
 
 // AMD-LABEL: define void @FuncOneMember(<2 x i32> %u.coerce)
 void FuncOneMember(struct StructOneMember u) {
   u.x = (int2)(0, 0);
 }
 
 // AMD-LABEL: define void @FuncOneLargeMember(%struct.LargeStructOneMember addrspace(5)* byval align 8 %u)
+// AMD-NOT: addrspacecast
+// AMD:   store <2 x i32> %{{.*}}, <2 x i32> addrspace(5)*
 void FuncOneLargeMember(struct LargeStructOneMember u) {
   u.x[0] = (int2)(0, 0);
 }
 
+// AMD20-LABEL: define void @test_indirect_arg_globl()
+// AMD20:  %[[byval_temp:.*]] = alloca %struct.LargeStructOneMember, align 8, addrspace(5)
+// AMD20:  %[[r0:.*]] = bitcast %struct.LargeStructOneMember addrspace(5)* %[[byval_temp]] to i8 addrspace(5)*
+// AMD20:  call void @llvm.memcpy.p5i8.p1i8.i64(i8 addrspace(5)* align 8 %[[r0]], i8 addrspace(1)* align 8 bitcast (%struct.LargeStructOneMember addrspace(1)* @g_s to i8 addrspace(1)*), i64 800, i1 false)
+// AMD20:  call void @FuncOneLargeMember(%struct.LargeStructOneMember addrspace(5)* byval align 8 %[[byval_temp]])
+#if __OPENCL_C_VERSION__ >= 200
+void test_indirect_arg_globl(void) {
+  FuncOneLargeMember(g_s);
+}
+#endif
+
+// AMD-LABEL: define amdgpu_kernel void @test_indirect_arg_local()
+// AMD: %[[byval_temp:.*]] = alloca %struct.LargeStructOneMember, align 8, addrspace(5)
+// AMD: %[[r0:.*]] = bitcast %struct.LargeStructOneMember addrspace(5)* %[[byval_temp]] to i8 addrspace(5)*
+// AMD: call void @llvm.memcpy.p5i8.p3i8.i64(i8 addrspace(5)* align 8 %[[r0]], i8 addrspace(3)* align 8 bitcast (%struct.LargeStructOneMember addrspace(3)* @test_indirect_arg_local.l_s to i8 addrspace(3)*), i64 800, i1 false)
+// AMD: call void @FuncOneLargeMember(%struct.LargeStructOneMember addrspace(5)* byval align 8 %[[byval_temp]])
+kernel void test_indirect_arg_local(void) {
+  local struct LargeStructOneMember l_s;
+  FuncOneLargeMember(l_s);
+}
+
+// AMD-LABEL: define void @test_indirect_arg_private()
+// AMD: %[[p_s:.*]] = alloca %struct.LargeStructOneMember, align 8, addrspace(5)
+// AMD-NOT: @llvm.memcpy
+// AMD-NEXT: call void @FuncOneLargeMember(%struct.LargeStructOneMember addrspace(5)* byval align 8 %[[p_s]])
+void test_indirect_arg_private(void) {
+  struct LargeStructOneMember p_s;
+  FuncOneLargeMember(p_s);
+}
+
 // AMD-LABEL: define amdgpu_kernel void @KernelOneMember
 // AMD-SAME:  (<2 x i32> %[[u_coerce:.*]])
 // AMD:  %[[u:.*]] = alloca %struct.StructOneMember, align 8, addrspace(5)
 // AMD:  %[[coerce_dive:.*]] = getelementptr inbounds %struct.StructOneMember, %struct.StructOneMember addrspace(5)* %[[u]], i32 0, i32 0
 // AMD:  store <2 x i32> %[[u_coerce]], <2 x i32> addrspace(5)* %[[coerce_dive]]
 // AMD:  call void @FuncOneMember(<2 x i32>
 kernel void KernelOneMember(struct StructOneMember u) {
   FuncOneMember(u);
void FuncTwoMember(struct StructTwoMember u) {
   u.y = (int2)(0, 0);
 }
 
 // AMD-LABEL: define void @FuncLargeTwoMember(%struct.LargeStructTwoMember addrspace(5)* byval align 8 %u)
 void FuncLargeTwoMember(struct LargeStructTwoMember u) {
   u.y[0] = (int2)(0, 0);
 }
 
-
 // AMD-LABEL: define amdgpu_kernel void @KernelTwoMember
 // AMD-SAME:  (%struct.StructTwoMember %[[u_coerce:.*]])
 // AMD:  %[[u:.*]] = alloca %struct.StructTwoMember, align 8, addrspace(5)
 // AMD: %[[LD0:.*]] = load <2 x i32>, <2 x i32> addrspace(5)*
 // AMD: %[[LD1:.*]] = load <2 x i32>, <2 x i32> addrspace(5)*
 // AMD: call void @FuncTwoMember(<2 x i32> %[[LD0]], <2 x i32> %[[LD1]])
 kernel void KernelTwoMember(struct StructTwoMember u) {
   FuncTwoMember(u);

test/CodeGenOpenCL/byval.cl

 // RUN: %clang_cc1 -emit-llvm -o - -triple amdgcn %s | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -o - -triple amdgcn---opencl %s | FileCheck %s
 
 struct A {
   int x[100];
 };
 
 int f(struct A a);
 
 int g() {
   struct A a;
   // CHECK: call i32 @f(%struct.A addrspace(5)* byval{{.*}}%a)
   return f(a);
 }
 
 // CHECK: declare i32 @f(%struct.A addrspace(5)* byval{{.*}})