Evan Cheng
2012-May-17 00:30 UTC
[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
Justin Holewinski
2012-May-18 18:51 UTC
[LLVMdev] [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
Justin Holewinski
2012-May-23 17:38 UTC
[LLVMdev] [llvm-commits] [PATCH][RFC] Add extra arguments to TargetLowering::LowerCall() so targets have more context in which to construct call chains
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>
Reasonably Related Threads
- [LLVMdev] [llvm-commits] [PATCH][RFC] Add extra arguments to TargetLowering::LowerCall() so targets have more context in which to construct call chains
- [LLVMdev] [llvm-commits] [PATCH][RFC] Add extra arguments to TargetLowering::LowerCall() so targets have more context in which to construct call chains
- [LLVMdev] [llvm-commits] [PATCH][RFC] Add extra arguments to TargetLowering::LowerCall() so targets have more context in which to construct call chains
- [LLVMdev] [llvm-commits] [PATCH][RFC] Add extra arguments to TargetLowering::LowerCall() so targets have more context in which to construct call chains
- [LLVMdev] [llvm-commits] [PATCH][RFC] Add extra arguments to TargetLowering::LowerCall() so targets have more context in which to construct call chains