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

[Justin Holewinski]

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