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

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

Evan

On Apr 19, 2012, at 8:07 AM, Justin Holewinski <jholewinski at
nvidia.com<mailto:jholewinski at nvidia.com>> wrote:


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

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

<0001-Add-extra-arguments-to-TargetLowering-LowerCall-so-t.patch>_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120419/54b59a25/attachment.html>

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.

Evan
>  
> Evan
>  
> On Apr 19, 2012, at 8:07 AM, Justin Holewinski <jholewinski at
nvidia.com> wrote:
> 
> 
> 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
>  
> 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.
>  
>
<0001-Add-extra-arguments-to-TargetLowering-LowerCall-so-t.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>  
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120419/e2731af2/attachment.html>

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