llvm dev - Jan 2018 - Options for custom CCState, CCAssignFn, and GlobalISel

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?
> 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?

Best,

Alex

> 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

> -----Original Message-----
> From: daniel_l_sanders at apple.com [mailto:daniel_l_sanders at apple.com]
> Sent: 05 January 2018 15:57
> To: Alex Bradbury
> Cc: llvm-dev; Leslie Zhai; Simon Dardis
> Subject: Re: [llvm-dev] Options for custom CCState, CCAssignFn, and
> GlobalISel
> 
> 
> 
> > 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.
Getting the original Type information would be ideal for Mips. The Original*
vectors that deal with vector types were to deal with vector arguments/returns
on MIPS. The particular quirk there was that vector arguments always go
through the integer register set, even if that vector is a legal type. 

I haven't had the bandwidth to track development in that area of GlobalISel.

Thanks.
Simon
> 
> > Best,
> >
> > Alex