[LLVMdev] Using CallingConvLower in ARM target
Sandeep Patel
deeppatel1987 at gmail.com
Tue Feb 17 16:42:43 PST 2009
This time with the test cases actually attached.
deep
On Tue, Feb 17, 2009 at 4:41 PM, Sandeep Patel <deeppatel1987 at gmail.com> wrote:
> On Mon, Feb 16, 2009 at 11:00 AM, Evan Cheng <evan.cheng at apple.com> wrote:
>> /// Information about how the value is assigned.
>> - LocInfo HTP : 7;
>> + LocInfo HTP : 6;
>>
>> Do you know why this change is needed? Are we running out of bits?
>
> HTP was't using all of these bits. I needed the hasCustom bit to come
> from somewhere unless we wanted to grow this struct, so I grabbed a
> bit from HTP.
>
>> - NeededStackSize = 4;
>> - break;
>> - case MVT::i64:
>> - case MVT::f64:
>> - if (firstGPR < 3)
>> - NeededGPRs = 2;
>> - else if (firstGPR == 3) {
>> - NeededGPRs = 1;
>> - NeededStackSize = 4;
>> - } else
>> - NeededStackSize = 8;
>> + State.addLoc(CCValAssign::getCustomMem(ValNo, ValVT,
>> + State.AllocateStack(4, 4),
>> + MVT::i32, LocInfo));
>> + return true; // we handled it
>>
>> Your change isn't handling the "NeededStackSize = 8" case.
>
> I believe it is. I've attached two additional test cases. The
> difference is that this case isn't handled by the CCCustomFns. They
> fail to allocate any regs and then handling falls through to an
> CCAssignToStack in ARMCallingConv.td. This is how other targets handle
> similar allocations.
>
>> ++ static const unsigned HiRegList[] = { ARM::R0, ARM::R2 };
>> + static const unsigned LoRegList[] = { ARM::R1, ARM::R3 };
>> +
>> + if (unsigned Reg = State.AllocateReg(HiRegList, LoRegList, 2)) {
>> + unsigned i;
>> + for (i = 0; i < 2; ++i)
>> + if (HiRegList[i] == Reg)
>> + break;
>> +
>> + State.addLoc(CCValAssign::getCustomReg(ValNo, ValVT, Reg,
>> + MVT::i32, LocInfo));
>> + State.addLoc(CCValAssign::getCustomReg(ValNo, ValVT, LoRegList[i],
>> + MVT::i32, LocInfo));
>>
>> Since 'i' is used after the loop, please choose a better variable name.
>>
>> Actually, is the loop necessary? We know the low register is always
>> one after the high register. Perhaps you can use
>> ARMRegisterInfo::getRegisterNumbering(Reg), add one to 1. And the
>> lookup the register enum with a new function (something like
>> getRegFromRegisterNum(RegNo, ValVT)).
>>
>> The patch is looking good. I need to run it through some more tests.
>> Unfortunately ARM target is a bit broken right now. I hope to fix it
>> today.
>
> I'll submit a revised patch after we've settled on the NeededStackSize=8 issue.
>
> deep
>
>> Thanks,
>>
>> Evan
>>
>> On Feb 13, 2009, at 8:27 PM, Sandeep Patel wrote:
>>
>>> Sorry left a small bit of cruft in ARMCallingConv.td. A corrected
>>> patch it attached.
>>>
>>> deep
>>>
>>> On Fri, Feb 13, 2009 at 6:41 PM, Sandeep Patel <deeppatel1987 at gmail.com
>>> > wrote:
>>>> Sure. Updated patches attached.
>>>>
>>>> deep
>>>>
>>>> On Fri, Feb 13, 2009 at 5:47 PM, Evan Cheng <evan.cheng at apple.com>
>>>> wrote:
>>>>>
>>>>> On Feb 13, 2009, at 4:25 PM, Sandeep Patel wrote:
>>>>>
>>>>>> ARMTargetLowering doesn't need case #1, but it seemed like you
>>>>>> and Dan
>>>>>> wanted a more generic way to inject C++ code into the process so I
>>>>>> tried to make the mechanism a bit more general.
>>>>>
>>>>> Ok. Since ARM doesn't need it and it's the only client, I'd much
>>>>> rather have CCCustomFn just return a single bool indicating
>>>>> whether it
>>>>> can handle the arg. Would that be ok?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Evan
>>>>>
>>>>>>
>>>>>>
>>>>>> deep
>>>>>>
>>>>>> On Fri, Feb 13, 2009 at 2:34 PM, Evan Cheng <evan.cheng at apple.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Feb 13, 2009, at 2:20 PM, Sandeep Patel wrote:
>>>>>>>
>>>>>>>> On Fri, Feb 13, 2009 at 12:33 PM, Evan Cheng <evan.cheng at apple.com
>>>>>>>> >
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> On Feb 12, 2009, at 6:21 PM, Sandeep Patel wrote:
>>>>>>>>>
>>>>>>>>>> Although it's not generally needed for ARM's use of CCCustom, I
>>>>>>>>>> return
>>>>>>>>>> two bools to handle the four possible outcomes to keep the
>>>>>>>>>> mechanism
>>>>>>>>>> flexible:
>>>>>>>>>>
>>>>>>>>>> * if CCCustomFn handled the arg or not
>>>>>>>>>> * if CCCustomFn wants to end processing of the arg or not
>>>>>>>>>
>>>>>>>>> +/// CCCustomFn - This function assigns a location for Val,
>>>>>>>>> possibly
>>>>>>>>> updating
>>>>>>>>> +/// all args to reflect changes and indicates if it handled
>>>>>>>>> it. It
>>>>>>>>> must set
>>>>>>>>> +/// isCustom if it handles the arg and returns true.
>>>>>>>>> +typedef bool CCCustomFn(unsigned &ValNo, MVT &ValVT,
>>>>>>>>> + MVT &LocVT, CCValAssign::LocInfo
>>>>>>>>> &LocInfo,
>>>>>>>>> + ISD::ArgFlagsTy &ArgFlags, CCState
>>>>>>>>> &State,
>>>>>>>>> + bool &result);
>>>>>>>>>
>>>>>>>>> Is "result" what you refer to as "isCustom" in the comments?
>>>>>>>>>
>>>>>>>>> Sorry, I am still confused. You mean it could return true but
>>>>>>>>> set
>>>>>>>>> 'result' to false? That means it has handled the argument but it
>>>>>>>>> would
>>>>>>>>> not process any more arguments? What scenario do you envision
>>>>>>>>> that
>>>>>>>>> this will be useful? I'd rather keep it simple.
>>>>>>>>
>>>>>>>> As you note there are three actual legitimate cases (of the four
>>>>>>>> combos):
>>>>>>>>
>>>>>>>> 1. The CCCustomFn wants the arg handling to proceed. This might
>>>>>>>> be
>>>>>>>> used akin to CCPromoteToType.
>>>>>>>> 2. The CCCustomFn entirely handled the arg. This might be used
>>>>>>>> akin to
>>>>>>>> CCAssignToReg.
>>>>>>>> 3. The CCCustomFn tried to handle the arg, but failed.
>>>>>>>>
>>>>>>>> these results are conveyed the following ways:
>>>>>>>>
>>>>>>>> 1. The CCCustomFn returns false, &result is not used.
>>>>>>>> 2. The CCCustomFn returns true, &result is false;
>>>>>>>> 3. The CCCustomFn returns true, &result is true.
>>>>>>>
>>>>>>> I don't think we want to support #1. If the target want to add
>>>>>>> custom
>>>>>>> code to handle an argument, if should be responsible for
>>>>>>> outputting
>>>>>>> legal code. Is there an actual need to support #1?
>>>>>>>
>>>>>>> Evan
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I tried to keep these CCCustomFns looking like TableGen generated
>>>>>>>> code. Suggestions of how to reorganize these results are
>>>>>>>> welcome. :-)
>>>>>>>> Perhaps better comments around the typedef for CCCustomFn would
>>>>>>>> suffice?
>>>>>>>>
>>>>>>>> The isCustom flag is simply a means for this machinery to
>>>>>>>> convey to
>>>>>>>> the TargetLowering functions to process this arg specially. It
>>>>>>>> may
>>>>>>>> not
>>>>>>>> always be possible for the TargetLowering functions to determine
>>>>>>>> that
>>>>>>>> the arg needs special handling after all the changes made by the
>>>>>>>> CCCustomFn or CCPromoteToType and other transformations.
>>>>>>>>
>>>>>>>>>> I placed the "unsigned i" outside those loops because i is used
>>>>>>>>>> after
>>>>>>>>>> the loop. If there's a better index search pattern, I'd be
>>>>>>>>>> happy
>>>>>>>>>> to
>>>>>>>>>> change it.
>>>>>>>>>
>>>>>>>>> Ok.
>>>>>>>>>
>>>>>>>>> One more nitpick:
>>>>>>>>>
>>>>>>>>> +/// CCCustom - calls a custom arg handling function
>>>>>>>>>
>>>>>>>>> Please capitalize "calls" and end with a period.
>>>>>>>>
>>>>>>>> Once we settle on the result handling changes, I'll submit an
>>>>>>>> update
>>>>>>>> with this change.
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Evan
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Attached is an updated patch against HEAD that has DebugLoc
>>>>>>>>>> changes. I
>>>>>>>>>> also split out the ARMAsmPrinter fix into it's own patch.
>>>>>>>>>>
>>>>>>>>>> deep
>>>>>>>>>>
>>>>>>>>>> On Mon, Feb 9, 2009 at 8:54 AM, Evan Cheng <echeng at apple.com>
>>>>>>>>>> wrote:
>>>>>>>>>>> Thanks Sandeep. I did a quick scan, this looks really good.
>>>>>>>>>>> But I
>>>>>>>>>>> do
>>>>>>>>>>> have a question:
>>>>>>>>>>>
>>>>>>>>>>> +/// CCCustomFn - This function assigns a location for Val,
>>>>>>>>>>> possibly
>>>>>>>>>>> updating
>>>>>>>>>>> +/// all args to reflect changes and indicates if it handled
>>>>>>>>>>> it. It
>>>>>>>>>>> must set
>>>>>>>>>>> +/// isCustom if it handles the arg and returns true.
>>>>>>>>>>> +typedef bool CCCustomFn(unsigned &ValNo, MVT &ValVT,
>>>>>>>>>>> + MVT &LocVT, CCValAssign::LocInfo
>>>>>>>>>>> &LocInfo,
>>>>>>>>>>> + ISD::ArgFlagsTy &ArgFlags, CCState
>>>>>>>>>>> &State,
>>>>>>>>>>> + bool &result);
>>>>>>>>>>>
>>>>>>>>>>> Is it necessary to return two bools (the second is returned by
>>>>>>>>>>> reference in 'result')? I am confused about the semantics of
>>>>>>>>>>> 'result'.
>>>>>>>>>>>
>>>>>>>>>>> Also, a nitpick:
>>>>>>>>>>>
>>>>>>>>>>> + unsigned i;
>>>>>>>>>>> + for (i = 0; i < 4; ++i)
>>>>>>>>>>>
>>>>>>>>>>> The convention we use is:
>>>>>>>>>>>
>>>>>>>>>>> + for (unsigned i = 0; i < 4; ++i)
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Evan
>>>>>>>>>>>
>>>>>>>>>>> On Feb 6, 2009, at 6:02 PM, Sandeep Patel wrote:
>>>>>>>>>>>
>>>>>>>>>>>> I think I've got all the cases handled now, implementing with
>>>>>>>>>>>> CCCustom<"foo"> callbacks into C++.
>>>>>>>>>>>>
>>>>>>>>>>>> This also fixes a crash when returning i128. I've also
>>>>>>>>>>>> included a
>>>>>>>>>>>> small asm constraint fix that was needed to build newlib.
>>>>>>>>>>>>
>>>>>>>>>>>> deep
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, Jan 19, 2009 at 10:18 AM, Evan Cheng
>>>>>>>>>>>> <evan.cheng at apple.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Jan 16, 2009, at 5:26 PM, Sandeep Patel wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Sat, Jan 3, 2009 at 11:46 AM, Dan Gohman <gohman at apple.com
>>>>>>>>>>>>>> >
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> One problem with this approach is that since i64 isn't
>>>>>>>>>>>>>>> legal,
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> bitcast would require custom C++ code in the ARM target to
>>>>>>>>>>>>>>> handle properly. It might make sense to introduce
>>>>>>>>>>>>>>> something
>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> CCIfType<[f64], CCCustom>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> where CCCustom is a new entity that tells the calling
>>>>>>>>>>>>>>> convention
>>>>>>>>>>>>>>> code to to let the target do something not easily
>>>>>>>>>>>>>>> representable
>>>>>>>>>>>>>>> in the tablegen minilanguage.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am thinking that this requires two changes: add a flag to
>>>>>>>>>>>>>> CCValAssign (take a bit from HTP) to indicate isCustom
>>>>>>>>>>>>>> and a
>>>>>>>>>>>>>> way
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>> author an arbitrary CCAction by including the source
>>>>>>>>>>>>>> directly in
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> TableGen mini-language. This latter change might want a
>>>>>>>>>>>>>> generic
>>>>>>>>>>>>>> change
>>>>>>>>>>>>>> to the TableGen language. For example, the syntax might be
>>>>>>>>>>>>>> like:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> class foo : CCCustomAction {
>>>>>>>>>>>>>> code <<< EOF
>>>>>>>>>>>>>> ....multi-line C++ code goes here that allocates regs & mem
>>>>>>>>>>>>>> and
>>>>>>>>>>>>>> sets CCValAssign::isCustom....
>>>>>>>>>>>>>> EOF
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Does this seem reasonable? An alternative is for CCCustom
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>> take a
>>>>>>>>>>>>>> string that names a function to be called:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> CCIfType<[f64], CCCustom<"MyCustomLoweringFunc">>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> the function signature for such functions will have to
>>>>>>>>>>>>>> return
>>>>>>>>>>>>>> two
>>>>>>>>>>>>>> results: if the CC processing is finished and if it the
>>>>>>>>>>>>>> func
>>>>>>>>>>>>>> succeeded
>>>>>>>>>>>>>> or failed:
>>>>>>>>>>>>>
>>>>>>>>>>>>> I like the second solution better. It seems rather
>>>>>>>>>>>>> cumbersome
>>>>>>>>>>>>> to
>>>>>>>>>>>>> embed
>>>>>>>>>>>>> multi-line c++ code in td files.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Evan
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> typedef bool CCCustomFn(unsigned ValNo, MVT ValVT,
>>>>>>>>>>>>>> MVT LocVT, CCValAssign::LocInfo LocInfo,
>>>>>>>>>>>>>> ISD::ArgFlagsTy ArgFlags, CCState &State,
>>>>>>>>>>>>>> bool &result);
>>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>>> LLVM Developers mailing list
>>>>>>>>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>>>>>>>
>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>> LLVM Developers mailing list
>>>>>>>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>>>>>>>
>>>>>>>>>>>> <
>>>>>>>>>>>> arm_callingconv
>>>>>>>>>>>> .diff>_______________________________________________
>>>>>>>>>>>> LLVM Developers mailing list
>>>>>>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>>>>>
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> LLVM Developers mailing list
>>>>>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>>>>>
>>>>>>>>>> <
>>>>>>>>>> arm_callingconv
>>>>>>>>>> .diff
>>>>>>>>>>> <
>>>>>>>>>>> arm_fixes.diff>_______________________________________________
>>>>>>>>>> LLVM Developers mailing list
>>>>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> LLVM Developers mailing list
>>>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> LLVM Developers mailing list
>>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> LLVM Developers mailing list
>>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>
>>>>>> _______________________________________________
>>>>>> LLVM Developers mailing list
>>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>
>>>>> _______________________________________________
>>>>> LLVM Developers mailing list
>>>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>
>>>>
>>> <
>>> arm_callingconv
>>> .diff><arm_fixes.diff>_______________________________________________
>>> LLVM Developers mailing list
>>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm_stack64_tests.diff
Type: application/octet-stream
Size: 1085 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090217/1d0dd739/attachment.obj>
More information about the llvm-dev
mailing list