All,
The attached patch adds two extra arguments to TargetLowering::LowerCall: RetTy and Args. These arguments are used in TargetLowering::LowerCallTo() to construct the Ins and OutVals parameters, but are not available to the target via LowerCall(). Some targets require this additional information, and the LowerCallTo() method is not virtual in TargetLowering. Instead of making that method virtual, this patch adds the extra arguments so targets needing this information do not need to replace LowerCallTo() which would lead to a lot of code duplication. While this does change the API for targets, existing targets can just safely ignore these parameters. This patch updates the LowerCall() prototypes for all in-tree targets.
We would like to get the community’s feedback on this so as to make sure this patch is as universally applicable as possible. While not currently used in the open-source tree, this patch lays some groundwork for submission of the NVIDIA NVPTX back-end into the open-source tree.
Thanks,
Justin Holewinski
0001-Add-extra-arguments-to-TargetLowering-LowerCall-so-t.patch (18.9 KB)
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?
Evan
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)?
Evan
From: Evan Cheng [mailto:evan.cheng@apple.com]
Sent: Thursday, April 19, 2012 10:47 AM
To: Justin Holewinski
Cc: llvmdev@cs.uiuc.edu; llvm-commits@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.
Evan
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
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.
llvm-lowercallto-refactor.patch (33.6 KB)
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
From: Evan Cheng [mailto:evan.cheng@apple.com]
Sent: Wednesday, May 16, 2012 5:30 PM
To: Justin Holewinski
Cc: llvmdev@cs.uiuc.edu; llvm-commits@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.
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?
0001-Change-interface-for-TargetLowering-LowerCallTo-and-.patch (66 KB)