[llvm-commits] [llvm-gcc-4.2] r48502 - /llvm-gcc-4.2/trunk/gcc/llvm-convert.cpp

[Duncan Sands]

Hi Dale,

> That term 'aggregate' is confusing.

inside llvm-convert, aggregate means: not a first class type.  So an
"aggregate" is exactly something that cannot be passed around in a
Value (thus the use of DestLoc).  Since gcc vector types are converted
to LLVM vector types, they are not aggregates in this sense, because
they are first class types.  In the rest of this email, when I use the
word "aggregate" I will mean it in this sense.

> From the point of view of LLVM   
> calls these vectors *are* aggregates in this case, in that the correct  
> IR to generate for the call uses the sret mechanism.

Let's call this: "pass by sret".  Whether something is passed by sret
or not is specified by the ABI and as such has nothing to do with whether
it is an aggregate or not.  For example, some strange ABI could specify
that integers should be passed using the sret mechanism.  Silly, but
logically possible.

> The gcc code,   
> however, does not think they are aggregates, and therefore generates  
> GCC SSA temporaries for these types.

LLVM puts them in registers too, so gcc and LLVM agree here.

> llvm-convert is making the   
> implicit assumption that gcc aggregates are the same thing as LLVM  
> aggregates, which is not true in this case.  Hence the problem.

Not sure what you mean here.  I guess you are saying: llvm-convert
assumes that pass-by-sret types are aggregate types (i.e. not first
class).  This is true, but only because it didn't come up before.

> EmitCallOf currently expects its caller to create and pass in the  
> return area for sret-using types, and does not return a value (such  
> functions are "void" in LLVM).

If the result of the call is not an aggregate, then EmitCallOf should
return the Value for it.  If it is an aggregate then it should arrange
for it to be stored to DestLoc.  This has nothing to do with whether
it is a pass-by-sret type or not, which is an ABI issue.  As such
EmitCallOf is not expecting DestLoc to be set for pass-by-sret types,
it is expecting it to be set for aggregates.

> My patch adjusts the code that calls   
> EmitCall (via Emit) to do that for this case.  Are you suggesting  
> changing this interface?

Not sure what you mean here.

> I guess EmitCallOf could be changed to   
> generate a return area and return a Load from it...

Exactly, this is what it should do for an non-aggregate type that is
pass-by-sret.

> if Loads of arbitrary aggregates work?

You only need to do this for non-aggregate pass-by-sret types, which
by definition can be loaded (because non-aggregate=first class).  If
the call returns an aggregate type then it needs to be placed in
DestLoc.

> There are a lot of places that pass in a   
> return area that would have to change, though.

Nope, none of them would have to change.

> > Presumably Emit is only returning 0 because it was provided with  
> > DestLoc.
> 
> No.  You mean "not provided with DestLoc?"
>
> > So if you make the modification I requested above, the code here  
> > presumably would
> > no longer be needed.
> >
> >> +    Value *RHS;
> >> +    tree fndecl;
> >> +    if (TREE_CODE(rhs)==CALL_EXPR &&
> >> +        TREE_CODE(TREE_TYPE(rhs))==VECTOR_TYPE &&
> >> +        LLVM_SHOULD_RETURN_VECTOR_AS_SHADOW(TREE_TYPE(rhs),
> >> +          ((fndecl = get_callee_fndecl(rhs)) ?  
> >> DECL_BUILT_IN(fndecl) :
> >> +                                               false))) {
> >> +      bool isVolatile = TREE_THIS_VOLATILE(lhs);
> >> +      unsigned Alignment = expr_align(lhs) / 8;
> >> +      MemRef NewLoc = CreateTempLoc(ConvertType(TREE_TYPE(rhs)));
> >> +      Emit(rhs, &NewLoc);
> >> +      LoadInst *LI = Builder.CreateLoad(NewLoc.Ptr, isVolatile,  
> >> "tmp");
> >> +      LI->setAlignment(Alignment);
> >> +      RHS = LI;
> >> +    } else {
> >> +      RHS = Emit(rhs, 0);
> >> +    }
> >>     RHS = CastToAnyType(RHS, RHSSigned,  
> >> ConvertType(TREE_TYPE(lhs)), LHSSigned);
> >>     SET_DECL_LLVM(lhs, RHS);
> >>     return RHS;

For example you wouldn't have to do this anymore.

> > Please don't break Emit invariants, it is only asking for trouble.

Just to repeat: the invariant is that aggregate types (= not first class)
get shoved into DestLoc, and non-aggregate types (= first class) get returned
as a Value.  This is how all the Emit machinery is structured, and EmitCallOf
should not be an exception.

Ciao,

Duncan.