[LLVMdev] Using CallingConvLower in ARM target
Evan Cheng
echeng at apple.com
Thu Feb 26 15:53:18 PST 2009
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
More information about the llvm-dev
mailing list