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

Wed May 23 10:38:12 PDT 2012

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>