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 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. 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 >-------------- next part -------------- A non-text attachment was scrubbed... Name: arm_callingconv.diff Type: application/octet-stream Size: 55367 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090212/31616686/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/20090212/31616686/attachment-0001.obj>
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.> > > 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. 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
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 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 >