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>
Sorry I haven't gotten back to you earlier. I have been busy. I ran some MultiSource/Benchmark earlier today. Looks like there are some failures: Fhourstones-3.1, Fhourstones, McCat/08-main, MiBench/ consumer-lame, Olden/Power, Olden/voronoi, mafft/pairlocalign, and sim. Are you able to test them on your end? Evan On Feb 17, 2009, at 4:42 PM, Sandeep Patel wrote:> 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 >>> >> > < > arm_stack64_tests.diff>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
I'm not currently setup to be able to run the A/B comparison tests that test-suite relies upon. Fhourstones-3.1 looks to be the simplest. If you can send me the two .o files from either EABI or Darwin, I can dig into why this went wrong for you. deep On Thu, Feb 26, 2009 at 3:53 PM, Evan Cheng <echeng at apple.com> wrote:> Sorry I haven't gotten back to you earlier. I have been busy. > > I ran some MultiSource/Benchmark earlier today. Looks like there are > some failures: Fhourstones-3.1, Fhourstones, McCat/08-main, MiBench/ > consumer-lame, Olden/Power, Olden/voronoi, mafft/pairlocalign, and > sim. Are you able to test them on your end? > > Evan > > On Feb 17, 2009, at 4:42 PM, Sandeep Patel wrote: > >> 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 >>>> >>> >> < >> arm_stack64_tests.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 >