arsenm added a comment.

In D79744#2030535 <https://reviews.llvm.org/D79744#2030535>, @rjmccall wrote:

> In D79744#2030481 <https://reviews.llvm.org/D79744#2030481>, @arsenm wrote:
>
> > In D79744#2030294 <https://reviews.llvm.org/D79744#2030294>, @rjmccall 
> > wrote:
> >
> > > The parameter variable is formally considered to be in a particular 
> > > address space, and taking the address of it yields a pointer in that 
> > > address space.  That can only work for an indirect argument if either (1) 
> > > the address space of the pointer that's actually passed can be freely 
> > > converted to that formal address space (because it's a subspace, or 
> > > because it's a superspace but known to be in the right subspace) or (2) 
> > > we're willing to copy the object into the right address space on the 
> > > callee side (and able to — obviously this only works for POD types).  I 
> > > do see the merit of allowing an address space to be specified for targets 
> > > that consider locals to be in a different formal address space than the 
> > > stack would naturally be (e.g. a generic address space); I don't remember 
> > > if that applies to AMDGPU.
> >
> >
> > Kernel arguments aren't directly visible to the user program, so this is an 
> > implementation detail. The user variable is the alloca that we need to 
> > explicitly copy to as you mentioned, which this patch does.
>
>
> It's usually a goal of indirect arguments to not need this copy.  We usually 
> bind parameters directly to the pointer that was passed in.
>
> > It's possible to poke at these through intrinsics, but the kernel address 
> > space isn't part of the language. POD type or not, we're going to have to 
> > do something to unload values from the constant buffer onto the stack.
> > 
> >> `byval` in LLVM is not a generic "this is a by-value argument" annotation; 
> >> it specifically means that the value is actually passed on the stack 
> >> despite the formally indirect convention, and therefore there's an 
> >> implicit memcpy on the caller side.  That is why `byval` is inherently 
> >> tied to the `alloca` address space: there's no actual pointer being 
> >> passed, so it doesn't make sense to pretend it might have been promoted 
> >> into a different address space.  That is also why there is no restriction 
> >> to writing into the pointer: there's no reason to prevent writing to the 
> >> argument slot.
> > 
> > In this case there is never a call. Only the callee read exists as this is 
> > the entry point. What would the alternative be? Add another flavor of byval 
> > attribute that's nearly identical?
>
> It's hard to answer this because I'm not sure what you're trying to 
> accomplish by marking the arguments `byval`.


The short answer is I'm trying to avoid fighting the optimizer's handling of 
aggregate load and stores. Passes already understand byval, but are actively 
harmful to aggregate loads and stores. Having explicit loads in the IR also 
brings it closer to what these are ultimately lowered to. Currently we emit a 
raw struct argument, which produces an aggregate store to alloca. Both SROA and 
instcombine unhelpfully split up the aggregate store, which doesn't optimize 
nicely like the memcpy from constant memory to alloca. The end result is we end 
up with more allocas and copies from constant memory that could have just read 
directly from the constant pointer.

The most honest calling convention handling would be to have an explicit 
constant pointer that all arguments are read from, and the function would have 
an empty argument list. This is somewhat impractical, since we still need to 
track the argument sizes and offsets somehow in the IR. It also becomes much 
more difficult to emit nicely annotated IR, since things like noalias still 
largely expect to be attached to a function argument. We do have an 
optimization pass that rewrites the arguments to look like this to enable 
vectorization later in the backend, but it suffers from losing useful alias 
annotations.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79744/new/

https://reviews.llvm.org/D79744



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to