llvm dev - May 2012 - [LLVMdev] [llvm-commits] [PATCH][RFC] Add extra arguments to TargetLowering::LowerCall() so targets have more context in which to construct call chains

Thanks. This is going in the right direction IMHO. Obviously, please make sure
you add comments to the data structure and convert all the targets first. Also,
please don't forget to send an email to llvmdev to warn owners of all the
out-of-tree targets about the ABI change.

Evan

On May 15, 2012, at 6:56 AM, Justin Holewinski <jholewinski at nvidia.com>
wrote:
> In response to this discussion, I've prepared the attached patch as an
initial prototype for the LowerCall/LowerCallTo change.  All of the information
currently needed by the back-ends, and the extra information needed by the NVPTX
back-end, is now wrapped in a CallLoweringInfo struct.  The various SelectionDAG
classes have been modified so any calls to TargetLowering::LowerCallTo use this
struct, and this struct is the means for passing information to/from each
target's LowerCall method.  Right now, the interface is a bit rough around
the edges, but I wanted to get feedback from the community on what you want in
this interface before spending too much time on it.  This patch currently only
addresses the X86 and NVPTX back-ends.  Once the interface is solidified, I will
update the rest of the in-tree targets.
> 
>> -----Original Message-----
>> From: Dan Gohman [mailto:gohman at apple.com]
>> Sent: Thursday, April 19, 2012 4:28 PM
>> To: Evan Cheng
>> Cc: Justin Holewinski; llvmdev at cs.uiuc.edu; Commit Messages and
Patches
>> for LLVM; Vinod Grover
>> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra arguments to
>> TargetLowering::LowerCall() so targets have more context in which to
>> construct call chains
>> 
>> 
>> On Apr 19, 2012, at 12:46 PM, Evan Cheng <evan.cheng at
apple.com> wrote:
>> 
>>> 
>>> On Apr 19, 2012, at 11:15 AM, Justin Holewinski wrote:
>>> 
>>>> 
>>>> From: Evan Cheng [mailto:evan.cheng at apple.com]
>>>> Sent: Thursday, April 19, 2012 10:47 AM
>>>> To: Justin Holewinski
>>>> Cc: llvmdev at cs.uiuc.edu; llvm-commits at cs.uiuc.edu; Vinod
Grover
>>>> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra arguments to
>> TargetLowering::LowerCall() so targets have more context in which to
>> construct call chains
>>>> 
>>>> TargetLowering::LowerCall is already a mess, I would really
prefer not to
>> extend it any further. It's especially difficult to justify
extending it without a
>> use in the open source tree.
>>>> 
>>>> We really should think hard about how to improve the API in two
ways.
>> Perhaps we should wrap the arguments in some struct rather than as
>> individual ones. We should also make it easier to extend it in the
future
>> without requiring an across the change for all in-tree and out-of-tree
targets.
>> Anyone wants to take the lead on this?
>>>> 
>>>> The second may be solved by the first.  If the LowerCall()
arguments are
>> encapsulated within some descriptor object, then extensions to this
>> descriptor class would be transparent to existing targets, both in-tree
and
>> out-of-tree (assuming the API is consistent).  What is the overall plan
for
>> LowerCall() and LowerCallTo()?  I noticed the comment on LowerCallTo()
that
>> implies it should be merged into SelectionDAGISel when all targets are
using
>> LowerCall (which I believe they are)?
>>> 
>>> I believe Dan has some thoughts in this topic.
>> 
>> That FIXME comment about LowerCallTo is probably an anachronism,
>> and no longer relevant given the changes that that code has been
>> through. LowerCallTo is currently a wrapper around LowerCall
>> which various things need. All targets which support calls are
>> using LowerCall.
>> 
>> Dan
> 
> 
>
-----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
contain
> confidential information.  Any unauthorized review, use, disclosure or
distribution
> is prohibited.  If you are not the intended recipient, please contact the
sender by
> reply email and destroy all copies of the original message.
>
-----------------------------------------------------------------------------------
>
<llvm-lowercallto-refactor.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

> -----Original Message-----
> From: Evan Cheng [mailto:evan.cheng at apple.com]
> Sent: Wednesday, May 16, 2012 5:30 PM
> To: Justin Holewinski
> Cc: llvmdev at cs.uiuc.edu; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra arguments to
> TargetLowering::LowerCall() so targets have more context in which to
> construct call chains
> 
> Thanks. This is going in the right direction IMHO. Obviously, please make
sure
> you add comments to the data structure and convert all the targets first.
> Also, please don't forget to send an email to llvmdev to warn owners of
all
> the out-of-tree targets about the ABI change.
Of course! :)

So, are people okay with the interface?  This is basically just struct-ifying
the parameters to TargetLowering::LowerCallTo/LowerCall.  My main concerns are
the handling of the InVals and IsTailCall variables.  I left InVals as a
separate parameter to LowerCall, since it is entirely produced by the
implementation.  IsTailCall remains in the struct since it is given a value
before LowerCall, although the LowerCall implementation can change its value
(hence not passing the struct by const-reference to LowerCall).  This feels
consistent enough for me, but I want to get the opinions of others before
actually finalizing the implementation and committing it.

Additionally, should the struct has getters/setters for the fields?  This seems
contrary to standard LLVM practice for structs, but I just want to clarify.
> 
> Evan
> 
> On May 15, 2012, at 6:56 AM, Justin Holewinski <jholewinski at
nvidia.com>
> wrote:
> 
> > In response to this discussion, I've prepared the attached patch
as an initial
> prototype for the LowerCall/LowerCallTo change.  All of the information
> currently needed by the back-ends, and the extra information needed by
> the NVPTX back-end, is now wrapped in a CallLoweringInfo struct.  The
> various SelectionDAG classes have been modified so any calls to
> TargetLowering::LowerCallTo use this struct, and this struct is the means
for
> passing information to/from each target's LowerCall method.  Right now,
the
> interface is a bit rough around the edges, but I wanted to get feedback
from
> the community on what you want in this interface before spending too much
> time on it.  This patch currently only addresses the X86 and NVPTX back-
> ends.  Once the interface is solidified, I will update the rest of the
in-tree
> targets.
> >
> >> -----Original Message-----
> >> From: Dan Gohman [mailto:gohman at apple.com]
> >> Sent: Thursday, April 19, 2012 4:28 PM
> >> To: Evan Cheng
> >> Cc: Justin Holewinski; llvmdev at cs.uiuc.edu; Commit Messages and
> Patches
> >> for LLVM; Vinod Grover
> >> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra arguments to
> >> TargetLowering::LowerCall() so targets have more context in which
to
> >> construct call chains
> >>
> >>
> >> On Apr 19, 2012, at 12:46 PM, Evan Cheng <evan.cheng at
apple.com>
> wrote:
> >>
> >>>
> >>> On Apr 19, 2012, at 11:15 AM, Justin Holewinski wrote:
> >>>
> >>>>
> >>>> From: Evan Cheng [mailto:evan.cheng at apple.com]
> >>>> Sent: Thursday, April 19, 2012 10:47 AM
> >>>> To: Justin Holewinski
> >>>> Cc: llvmdev at cs.uiuc.edu; llvm-commits at cs.uiuc.edu;
Vinod Grover
> >>>> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra
arguments to
> >> TargetLowering::LowerCall() so targets have more context in which
to
> >> construct call chains
> >>>>
> >>>> TargetLowering::LowerCall is already a mess, I would
really prefer not
> to
> >> extend it any further. It's especially difficult to justify
extending it without
> a
> >> use in the open source tree.
> >>>>
> >>>> We really should think hard about how to improve the API
in two ways.
> >> Perhaps we should wrap the arguments in some struct rather than as
> >> individual ones. We should also make it easier to extend it in the
future
> >> without requiring an across the change for all in-tree and
out-of-tree
> targets.
> >> Anyone wants to take the lead on this?
> >>>>
> >>>> The second may be solved by the first.  If the LowerCall()
arguments
> are
> >> encapsulated within some descriptor object, then extensions to
this
> >> descriptor class would be transparent to existing targets, both
in-tree and
> >> out-of-tree (assuming the API is consistent).  What is the overall
plan for
> >> LowerCall() and LowerCallTo()?  I noticed the comment on
LowerCallTo()
> that
> >> implies it should be merged into SelectionDAGISel when all targets
are
> using
> >> LowerCall (which I believe they are)?
> >>>
> >>> I believe Dan has some thoughts in this topic.
> >>
> >> That FIXME comment about LowerCallTo is probably an anachronism,
> >> and no longer relevant given the changes that that code has been
> >> through. LowerCallTo is currently a wrapper around LowerCall
> >> which various things need. All targets which support calls are
> >> using LowerCall.
> >>
> >> Dan
> >
> >
> >
-----------------------------------------------------------------------------------
> > This email message is for the sole use of the intended recipient(s)
and may
> contain
> > confidential information.  Any unauthorized review, use, disclosure or
> distribution
> > is prohibited.  If you are not the intended recipient, please contact
the
> sender by
> > reply email and destroy all copies of the original message.
> >
-----------------------------------------------------------------------------------
> > <llvm-lowercallto-
> refactor.patch>______________________________________________
> _
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Attached is the (hopefully) final version of this patch.  This updates all
in-tree targets, and does not introduce any regression test failures.

Okay to commit?
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Justin Holewinski
> Sent: Friday, May 18, 2012 11:51 AM
> To: Evan Cheng
> Cc: llvm-commits at cs.uiuc.edu; llvmdev at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra arguments to
> TargetLowering::LowerCall() so targets have more context in which to
> construct call chains
> 
> > -----Original Message-----
> > From: Evan Cheng [mailto:evan.cheng at apple.com]
> > Sent: Wednesday, May 16, 2012 5:30 PM
> > To: Justin Holewinski
> > Cc: llvmdev at cs.uiuc.edu; llvm-commits at cs.uiuc.edu
> > Subject: Re: [llvm-commits] [PATCH][RFC] Add extra arguments to
> > TargetLowering::LowerCall() so targets have more context in which to
> > construct call chains
> >
> > Thanks. This is going in the right direction IMHO. Obviously, please
make
> sure
> > you add comments to the data structure and convert all the targets
first.
> > Also, please don't forget to send an email to llvmdev to warn
owners of all
> > the out-of-tree targets about the ABI change.
> 
> Of course! :)
> 
> So, are people okay with the interface?  This is basically just
struct-ifying the
> parameters to TargetLowering::LowerCallTo/LowerCall.  My main concerns
> are the handling of the InVals and IsTailCall variables.  I left InVals as
a
> separate parameter to LowerCall, since it is entirely produced by the
> implementation.  IsTailCall remains in the struct since it is given a value
> before LowerCall, although the LowerCall implementation can change its
> value (hence not passing the struct by const-reference to LowerCall).  This
> feels consistent enough for me, but I want to get the opinions of others
> before actually finalizing the implementation and committing it.
> 
> Additionally, should the struct has getters/setters for the fields?  This
seems
> contrary to standard LLVM practice for structs, but I just want to clarify.
> 
> >
> > Evan
> >
> > On May 15, 2012, at 6:56 AM, Justin Holewinski <jholewinski at
nvidia.com>
> > wrote:
> >
> > > In response to this discussion, I've prepared the attached
patch as an
> initial
> > prototype for the LowerCall/LowerCallTo change.  All of the
information
> > currently needed by the back-ends, and the extra information needed by
> > the NVPTX back-end, is now wrapped in a CallLoweringInfo struct.  The
> > various SelectionDAG classes have been modified so any calls to
> > TargetLowering::LowerCallTo use this struct, and this struct is the
means for
> > passing information to/from each target's LowerCall method.  Right
now,
> the
> > interface is a bit rough around the edges, but I wanted to get
feedback
> from
> > the community on what you want in this interface before spending too
> much
> > time on it.  This patch currently only addresses the X86 and NVPTX
back-
> > ends.  Once the interface is solidified, I will update the rest of the
in-tree
> > targets.
> > >
> > >> -----Original Message-----
> > >> From: Dan Gohman [mailto:gohman at apple.com]
> > >> Sent: Thursday, April 19, 2012 4:28 PM
> > >> To: Evan Cheng
> > >> Cc: Justin Holewinski; llvmdev at cs.uiuc.edu; Commit
Messages and
> > Patches
> > >> for LLVM; Vinod Grover
> > >> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra arguments
to
> > >> TargetLowering::LowerCall() so targets have more context in
which to
> > >> construct call chains
> > >>
> > >>
> > >> On Apr 19, 2012, at 12:46 PM, Evan Cheng <evan.cheng at
apple.com>
> > wrote:
> > >>
> > >>>
> > >>> On Apr 19, 2012, at 11:15 AM, Justin Holewinski wrote:
> > >>>
> > >>>>
> > >>>> From: Evan Cheng [mailto:evan.cheng at apple.com]
> > >>>> Sent: Thursday, April 19, 2012 10:47 AM
> > >>>> To: Justin Holewinski
> > >>>> Cc: llvmdev at cs.uiuc.edu; llvm-commits at
cs.uiuc.edu; Vinod Grover
> > >>>> Subject: Re: [llvm-commits] [PATCH][RFC] Add extra
arguments to
> > >> TargetLowering::LowerCall() so targets have more context in
which to
> > >> construct call chains
> > >>>>
> > >>>> TargetLowering::LowerCall is already a mess, I would
really prefer not
> > to
> > >> extend it any further. It's especially difficult to
justify extending it
> without
> > a
> > >> use in the open source tree.
> > >>>>
> > >>>> We really should think hard about how to improve the
API in two
> ways.
> > >> Perhaps we should wrap the arguments in some struct rather
than as
> > >> individual ones. We should also make it easier to extend it
in the future
> > >> without requiring an across the change for all in-tree and
out-of-tree
> > targets.
> > >> Anyone wants to take the lead on this?
> > >>>>
> > >>>> The second may be solved by the first.  If the
LowerCall() arguments
> > are
> > >> encapsulated within some descriptor object, then extensions
to this
> > >> descriptor class would be transparent to existing targets,
both in-tree
> and
> > >> out-of-tree (assuming the API is consistent).  What is the
overall plan for
> > >> LowerCall() and LowerCallTo()?  I noticed the comment on
LowerCallTo()
> > that
> > >> implies it should be merged into SelectionDAGISel when all
targets are
> > using
> > >> LowerCall (which I believe they are)?
> > >>>
> > >>> I believe Dan has some thoughts in this topic.
> > >>
> > >> That FIXME comment about LowerCallTo is probably an
anachronism,
> > >> and no longer relevant given the changes that that code has
been
> > >> through. LowerCallTo is currently a wrapper around LowerCall
> > >> which various things need. All targets which support calls
are
> > >> using LowerCall.
> > >>
> > >> Dan
> > >
> > >
> > >
-----------------------------------------------------------------------------------
> > > This email message is for the sole use of the intended
recipient(s) and
> may
> > contain
> > > confidential information.  Any unauthorized review, use,
disclosure or
> > distribution
> > > is prohibited.  If you are not the intended recipient, please
contact the
> > sender by
> > > reply email and destroy all copies of the original message.
> > >
-----------------------------------------------------------------------------------
> > > <llvm-lowercallto-
> >
> refactor.patch>______________________________________________
> > _
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Change-interface-for-TargetLowering-LowerCallTo-and-.patch
Type: application/octet-stream
Size: 67630 bytes
Desc: 0001-Change-interface-for-TargetLowering-LowerCallTo-and-.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120523/65da0c99/attachment.obj>