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

[Sandeep Patel]

On Sat, Aug 15, 2009 at 3:20 PM, Bob Wilson<bob.wilson at apple.com> wrote:
>
> 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?

The patch covered everywhere in llvm-gcc, not LLVM proper. There are a
couple of wrinkles in doing the same to LLVM: the C API takes an
unsigned in LLVMSetFunctionCallConv() and LLParser needs to
ParseUInt32() in ParseOptionalCallingConv(). An hour before getting on
a plane doesn't seem like the time to submit a patch that changes the
API, so I think we can revisit the LLVM part of this cleanup later.

> 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.

Many thanks!

deep

>> 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>
>
>