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 >-------------- next part -------------- A non-text attachment was scrubbed... Name: arm_callingconv.diff Type: application/octet-stream Size: 54711 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090213/8cc9cfc6/attachment.obj> -------------- next part -------------- A non-text attachment was scrubbed... Name: arm_fixes.diff Type: application/octet-stream Size: 590 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090213/8cc9cfc6/attachment-0001.obj>
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 >> >-------------- next part -------------- A non-text attachment was scrubbed... Name: arm_callingconv.diff Type: application/octet-stream Size: 54395 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090213/387f69fd/attachment.obj> -------------- next part -------------- A non-text attachment was scrubbed... Name: arm_fixes.diff Type: application/octet-stream Size: 590 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090213/387f69fd/attachment-0001.obj>
Thanks.
More questions :-)
/// 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?
- 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.
++ 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.
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