[llvm-commits] calling convention support for ARM AAPCS VFP "homogenous aggregates"

[Bob Wilson]

On Aug 15, 2009, at 7:44 AM, Sandeep Patel wrote:

> I was using that GCC varargs code around the office as an example of
> why LLVM is easier to maintain than GCC. :-)
>
> Does build_gcc build with -Werror? That's not the norm for GCC's
> configure; all of the auxiliary projects like libstdc++ have a ton of
> warnings.

Yes, build_gcc uses -Werror.   Your patch fixes the warning, but I  
wasn't expecting to see a cast there -- I thought one of your patches  
changed it to use CallingConv::ID everywhere instead of unsigned.  Did  
you miss something or is there a reason to keep it an unsigned in some  
places?

I went ahead and committed the patches.  You've been waiting a while  
to get this reviewed, and I don't want to put it in right before the  
2.6 freeze.

>
> Attached is a small patch to address these. Thanks for reviewing this!
>
> deep
>
> On Sat, Aug 15, 2009 at 4:28 AM, Bob Wilson<bob.wilson at apple.com>  
> wrote:
>> Thanks for splitting it up.  I was all set to commit it for you,  
>> but it
>> fails to build for me because of a compiler warning (with -Werror  
>> enabled):
>>
>> gcc/llvm-convert.cpp: In member function 'void
>> TreeToLLVM::StartFunctionBody()':
>> gcc/llvm-convert.cpp:446: warning: comparison between signed and  
>> unsigned
>> integer expressions
>> gcc/llvm-convert.cpp:458: warning: comparison between signed and  
>> unsigned
>> integer expressions
>>
>> Those lines are the "Fn->getCallingConv() == CallingConv" assertions.
>> Can you investigate?
>>
>> Can you also write a comment for the first patch, both to explain  
>> that the
>> code is checking for variable arguments (GCC's representation of  
>> that is far
>> from obvious) and to explain why you're checking for it.  (You  
>> summarized it
>> nicely below but there's nothing in the code.)
>>
>> For future reference, for the llvm-specific C++ files in llvm-gcc,  
>> it seems
>> to me like the convention is to follow the usual llvm coding  
>> conventions.
>>  You had some C-style comments in llvm-arm.cpp. I've already gone  
>> through
>> and changed them to C++ comments for you, so you don't need to do  
>> anything
>> about it now.  I also found some typos and spelling issues (e.g.,
>> "homogenous" is usually spelled "homogeneous") that I will fix for  
>> you.
>>
>> Aside from those things the patch looks good to me, so I'll commit  
>> it for
>> you once we get past the build problem.
>>
>> On Aug 14, 2009, at 2:06 AM, Sandeep Patel wrote:
>>
>>> Attached are split versions of this patch as requested, with minor
>>> updates for the latest LLVM API changes applied.
>>>
>>> 1. deep-gcc-hard-float-varargs.diff applies the AAPCS base standard
>>> CallingConv for varargs functions, as per the spec.
>>> 2. deep-gcc-callingconv-id.diff is item 1 below, changing remaining
>>> uses of a CallingConv from unsigned to CallingConv::ID. The  
>>> additional
>>> header inclusion is very minimal, only in llvm-internal.h.
>>> 3. deep-gcc-calling-conv-mixed.diff is items 2 and 3 below, tracking
>>> the CC in all DefaultABIClient subclasses and flowing it into
>>> LLVM_SHOULD_PASS_AGGREGATE_IN_MIXED_REGS and
>>> LLVM_AGGREGATE_PARTIALLY_PASSED_IN_REGS. This adds an include of
>>> CallingConv.h in llvm-abi.h, which is already includes
>>> llvm-internal.h.
>>> 4. deep-gcc-homogeneous.diff is item 4 below, adding the use of this
>>> stuff in the ARM backend as per the AAPCS VFP spec.
>>>
>>> deep
>>>
>>> On Fri, Aug 14, 2009 at 5:27 AM, Bob Wilson<bob.wilson at apple.com>  
>>> wrote:
>>>>
>>>> Sandeep,
>>>>
>>>> This looks pretty good to me.  I have some minor comments that I  
>>>> will try
>>>> to
>>>> collect in the next day or two.  I think you should split this up  
>>>> a bit,
>>>> though.  Having seen the motivation for making the CC available  
>>>> in more
>>>> contexts, it would be good to submit that as a separate patch.   
>>>> I.E., one
>>>> patch for parts 2 and 3 from your list.
>>>>
>>>> The change to use CallingConv::ID should be reviewed by someone  
>>>> more
>>>> familiar with llvm-gcc.  It looks to me like someone intentionally
>>>> decided
>>>> to use "unsigned" to avoid pulling in the header file for
>>>> CallingConv::ID,
>>>> and now you're changing that decision.
>>>>
>>>> On Aug 5, 2009, at 3:15 AM, Sandeep Patel wrote:
>>>>
>>>>> Attached is a patch to llvm-gcc to implement passing of homogenous
>>>>> aggregates via FP registers per the ARM AAPCS VFP variant  
>>>>> (a.k.a. hard
>>>>> float).
>>>>>
>>>>> 1. The patch uses CallingConv::ID consistently instead of  
>>>>> unsigned to
>>>>> hold the CC.
>>>>> 2. The patch keeps track of the CC in all DefaultABIClient  
>>>>> subclasses.
>>>>> 3. The patch passes the CC into
>>>>> LLVM_SHOULD_PASS_AGGREGATE_IN_MIXED_REGS and
>>>>> LLVM_AGGREGATE_PARTIALLY_PASSED_IN_REGS.
>>>>> 4. The patch implements both macros in the ARM backend according  
>>>>> to
>>>>> the rules of the AAPCS spec.
>>>>>
>>>>> Note: no code was imported to implement this. The structure of
>>>>> vfp_arg_homogenous_aggregate_p was originally taken from the i386
>>>>> classify_argument() function.
>>>>>
>>>>> deep
>>>>>
>>>>>
>>>>> <deep-gcc-hard-float- 
>>>>> homogeneous.diff>_______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>> <deep-gcc-hard-float-varargs.diff><deep-gcc-callingconv- 
>>> id.diff><deep-gcc-calling-conv-mixed.diff><deep-gcc- 
>>> homogeneous.diff>
>>
>>
> <deep-gcc-cleanups.diff>