[llvm-dev] Options for custom CCState, CCAssignFn, and GlobalISel

[Daniel Sanders via llvm-dev]

> On 4 Jan 2018, at 10:51, Alex Bradbury <asb at lowrisc.org> wrote:
> 
> On 4 January 2018 at 17:10, Daniel Sanders via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>>> On 3 Jan 2018, at 14:00, Alex Bradbury via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>> I haven't dug into the GlobalISel calling convention code much but I can comment on the MipsCCState.
> 
> Thanks for the insight Daniel, much appreciated.
> 
>>> * MipsCCState: adds bool vectors OriginalArgWasF128, OriginalArgWasFloat,
>>> OriginalArgWasFloatVector, OriginalRetWasFloatVector, CallOperandIsFixed. Also
>>> a SpeciallCallingConv field. Provides its own implementation of
>>> AnalyzeFormalArguments etc that fill these vectors.
>> 
>> CallOperandIsFixed was needed because the CCIf* classes could tell whether the argument list was variable or not, but couldn't tell whether a particular argument was part of the variable portion of the argument list. At the time, it struck me as odd that this information wasn't part of ArgFlagsTy but I didn't want to add it there because it appeared to be full and I didn't want to make everyone have a 65-bit ArgFlagsTy just because of a Mips quirk. I see Reid Kleckner changed this object last year and found that it wasn't actually full so maybe we should move OutputArg::IsFixed into ArgFlagsTy now.
> 
> AArch64, Hexagon, RISCV, and SystemZ all have the same requirement.
> AArch64 works around it by calling its CCAssignFnForCall helper for
> every argument (and passing IsFixed through to that). For GISel, it
> overrides assignArg so a different function can be called for varargs.
> 
> I'd be in favour of adding IsFixed to ArgFlagsTy even if it did
> "overflow" ArgFlagsTy. I know there's an argument about "death by a
> thousand papercuts", but is the size of ArgFlagsTy really critical
> anyway?

Probably not, but someone had gone to a lot of trouble to manually pack it into 64-bits so I assumed there was a reason I couldn't see.

>> I think SpeciallCallingConv ought to be eliminated and Mips16's calling convention be given it's own CallingConv def instead. If I remember rightly, I left it there because I didn't want to change Mips16 at the time.
>> 
>> I'll comment on the OriginalArg* family below.
>> 
>>> * HexagonCCState: adds a single extra field - NumNamedVarArgParams.
>>> * PPCCCState: adds an OriginalArgWasPPCF128 bool vector. Arguably reduces
>>> boilerplate vs MipsCCState by just having PPCISelLowering call
>>> PPCCCState::PreAnalyzeCallOperands directly.
>>> * SystemZCCState: has bool vectors ArgIsFixed and ArgIsShortVector, works
>>> similarly to MipsCCState or PPCCCState.
>>> 
>>> The above works, but it isn't directly usable in the current GlobalISel
>>> implementation. Very sensibly, GISel tries to both reuse existing calling
>>> convention implementations and to reduce duplicated code as much as possible.
>>> To this end, CallLowering::handleAssignments will create a CCState and use
>>> ValueHandler::assignArg to call a function of type CCAssignFn type.
>>> 
>>> I see a couple of options:
>>> 1) Creating a new virtual method in GISel CallLowering that creates and
>>> initialises a CCState or custom subclass. Use that to support target-specific
>>> CCStates.
>>> 2) Try to remove the need for custom CCState altogether. In
>>> <https://reviews.llvm.org/D38894>, Shiva Chen adds an OrigVT field to
>>> ISD::ArgFlagsTy which seems much cleaner. It's not immediately obvious to me
>>> if the in-tree users that currently track Type rather than EVT could always
>>> make do with an EVT instead. [Input welcome!].
>> 
>> Unfortunately, this solution wouldn't work for Mips since the 'Original*' vectors provide information from the frontend before the type was lowered to a VT. For example, OriginalArgWasF128[I] can be true when the VT before legalization was not f128. One of these cases is a structure containing a 128-bit float which the frontend lowers to i128. The other is an i128 containing a softfloat representation of a 128-bit float. If I remember correctly, the need for the latter is caused by a gcc bug that became a de-facto part of the ABI by going unnoticed for ~10 years until we tried to link clang-compiled and gcc-compiled objects together and found it didn't work.
> 
> The question then is whether the backends that currently are happy
> with EVT (SystemZ, PPC) might be just has happy with Type. Thanks for
> pointing out that MipsCCState::originalTypeIsF128 is slightly more
> involved than other similar functions (checking for f128 libcall
> names).
> 
> It seems Mips has the most fiddly CCState, so perhaps the key question
> is: has anyone given any thought to supporting Mips in GlobalISel?

I'm trying to keep it in mind when we design things so we don't do anything that's too painful for Mips but I'm sure I'll miss things.

> Best,
> 
> Alex