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

[Dale Johannesen]

It's easy to recognize this code as ugly; I agree and I'm open to  
improving it.  But I looked at alternatives before doing it this way  
and I don't think it's that easy.

On Mar 19, 2008, at 1:34 AM, Duncan Sands wrote:

> Hi Dale,
>
>> Value *TreeToLLVM::Emit(tree exp, const MemRef *DestLoc) {
>> -  assert((isAggregateTreeType(TREE_TYPE(exp)) == (DestLoc != 0) ||
>> +  tree fndecl;
>> +  // Some vectors are returned using sret; these should have  
>> DestLoc provided.
>> +  assert(((isAggregateTreeType(TREE_TYPE(exp)) == (DestLoc != 0)) ||
>> +          (TREE_CODE(exp)==CALL_EXPR &&
>> +           TREE_CODE(TREE_TYPE(exp))==VECTOR_TYPE &&
>> +           LLVM_SHOULD_RETURN_VECTOR_AS_SHADOW(TREE_TYPE(exp),
>> +             ((fndecl = get_callee_fndecl(exp)) ?  
>> DECL_BUILT_IN(fndecl) :
>> +                                                false))) ||
>>           TREE_CODE(exp) == MODIFY_EXPR) &&
>
> rather than this ugliness in Emit, how about teaching the return  
> value code to
> handle it, by having it only introduce the DestLoc+Emit-to-DestLoc  
> when handling
> an aggregate, and making it generate an Emit-of-scalar+Store for a  
> scalar (in
> this case a vector type).  This is how everywhere else handles the  
> scalar vs
> aggregate distinction.

The "return value code" would be EmitCallOf?

That term 'aggregate' is confusing.  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.  The gcc code,  
however, does not think they are aggregates, and therefore generates  
GCC SSA temporaries for these types.  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.

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).   My patch adjusts the code that calls  
EmitCall (via Emit) to do that for this case.  Are you suggesting  
changing this interface?   I guess EmitCallOf could be changed to  
generate a return area and return a Load from it...if Loads of  
arbitrary aggregates work?   There are a lot of places that pass in a  
return area that would have to change, though.

>> -
>> -    Value *RHS = Emit(rhs, 0);
>> +    // Some types (so far, generic vectors on x86-32 and ppc32)  
>> can have
>> +    // GCC SSA temporaries for them, but are passed using sret,  
>> which means
>> +    // Emit will return 0.  Handle this.
>
> 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;
>
> Please don't break Emit invariants, it is only asking for trouble.
>
> Ciao,
>
> Duncan.