[llvm-commits] [PATCH] ARM AAPCS-VFP hard float homogeneous struct returns

[Bob Wilson]

Hi Sandeep, I've committed this.  I've got some follow-on changes that  
you should watch for.  See comments below.

On Oct 26, 2009, at 10:21 PM, Bob Wilson wrote:

> Nice!  Thanks for doing this.  Some comments/questions:
>
> * Typo: DESTFIELNO -> DESTFIELDNO in the following comment:
>
> +// llvm_arm_extract_mrv_array_element - Helper function that helps  
> extract
> +// an array element from multiple return value.
> +//
> +// Here, SRC is returning multiple values. DEST's DESTFIELNO field  
> is an array.
> +// Extract SRCFIELDNO's ELEMENO value and store it in DEST's  
> FIELDNO field's
> +// ELEMENTNO.

I fixed this for you.

>
> * Can you add an assertion at the beginning of  
> llvm_arm_extract_mrv_array_element to check that Src->getType() is a  
> StructType?

I did not fix this.

>
> * Likewise, at the beginning of  
> llvm_arm_extract_multiple_return_value, it would be good to assert  
> that all the types you cast match your expectations.

...nor this.  Please submit a patch to add the assertions.

>
> * Also in llvm_arm_extract_multiple_return_value, there is a comment:
>
> +    // Directly access first class values using getresult.
>
> but I don't see anything in the code involving "getresult".  Just  
> drop the "using getresult"?

I dropped it.

>
> * Is the following comment in  
> llvm_arm_should_pass_or_return_aggregate_in_regs still relevant with  
> the new containerized vector types?
>
> +  // Alas, we can't use LLVM Types to figure this out because we  
> need to
> +  // examine unions closely.  We'll have to walk the GCC TreeType.
>
> If not, please remove it.

Nevermind.  I think it relevant.  I was thinking that the unions in  
question were something internal to arm_neon.h that might have been  
removed.  It is really about all unions, though.

>
> * Next time keep the whitespace changes separate.  It makes it  
> harder to review when you combine them.
>
> I read through the patch pretty carefully and it looks good to me,  
> but I'd like to test it out.  I'll try to get to that tomorrow.

I tested it.  It doesn't work.  See follow-on patches.....

>
> On Oct 26, 2009, at 4:37 PM, Sandeep Patel wrote:
>
>> The attached patch to llvm-gcc-4.2 fixes incorrect returns of
>> homogeneous structures. Instead of using a shadow parameter they are
>> now register allocated.
>>
>> deep
>> <deep-gcc-homogeneous- 
>> returns.diff>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits