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

[Yaxun Liu via Phabricator via cfe-commits]

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


================
Comment at: lib/CodeGen/CGCall.cpp:3851
+                                         ->getType()
+                                         ->getPointerAddressSpace();
         const unsigned ArgAddrSpace =
----------------
rjmccall wrote:
> yaxunl wrote:
> > 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?
> 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.
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.




https://reviews.llvm.org/D34367