[llvm-commits] [llvm] r37940 - in /llvm/trunk: include/llvm/CodeGen/SelectionDAGNodes.h include/llvm/ParameterAttributes.h lib/AsmParser/Lexer.l lib/AsmParser/llvmAsmParser.y lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp lib/Target/TargetCallingConv.td lib/Target/X86/X86CallingConv.td lib/VMCore/Function.cpp lib/VMCore/Verifier.cpp utils/TableGen/CallingConvEmitter.cpp

[Duncan Sands]

Hi Reid,

> > > > +    ByVal             = 1<<4,  ///< Struct passed by value
> > > 
> > > I would prefer this be named StructByVal, to be clear
> > 
> > this was changed because it should also be possible to pass arrays
> > by value, not just structs.  So it is rather the comment that should
> > be fixed, not the name.  That said, ByVal is a little confusing since
> > eg an integer is passed by value but is not marked ByVal.  Not clear
> > what a better name would be...
> 
> Okay, so now I'm really confused. :)
> 
> The Verifier is checking to make sure that the argument type to which
> ByVal is applied is a pointer. It doesn't check any further so I assume
> a pointer to anything is valid. So, consider the following:
> 
> 1. i32*              ; i32 is passed?
> 2. i1024*            ; We pass a 128-byte long integer?
> 3. f64*              ; We pass a double?
> 4. {i32,i32,i32}*    ; We pass 3 i32 integers?
> 5. <4xi32>*          ; We pass 4 i32 integers?
> 6. [1024xi32>*       ; We pass 1024 i32 integers?
> 
> >From an assembly language point of view, this seems redundant with
> existing capabilities as well as violating one of the tenets of the
> language (can only pass first class types).  
> 
> In cases 1, 3, 4 and 5 the front end could have just done the
> getelementptr and load(s) necessary. For cases 2 and 6, the implied
> loads are a performance issue so it would be better to pass the pointer.
> Also, in all these cases, the implied signature of the function is
> different. How does this play with calling conventions?
> 
> Am I missing something here? Because if I'm not then I'm -1 on this
> feature as a whole. What's the objective of this?

first let me point out that much of what you say also applies to
StructReturn, which probably can also be used for returning arrays,
ints etc.  And like StructReturn this is all about the ABI: my
understanding is that on some platforms the ABI allows for passing
(small) structs by value.  We need to be able to support this, but
how?  The idea is to treat such parameters as passed by reference,
and add an attribute to tell the code generators to in fact pass them
in whatever way the ABI says they should be.  See PR1521.

Thus in the IR the function is declared to take a pointer to the
struct as an argument, and the ByVal marker is placed for the
benefit of the code generators.  However it is not clear to me
that this is semantically correct.  Doesn't ByVal mean what it
says: a copy of the struct is made, and if this copy is modified
inside the called function then this does not change the original
struct?  But if in the IR we pass in a pointer to the struct, then
any writes to the struct will modify the original, which is wrong.

Also, I agree that there needs to be more clarity about what is
allowed to be passed ByVal and returned by SRet.

Finally, if StructReturn can also return arrays, then I don't mind
to much if ByVal is called StructByVal or something like that.

Ciao,

Duncan.