llvm dev - May 2012 - [LLVMdev] [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

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.
-----------------------------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-lowercallto-refactor.patch
Type: application/octet-stream
Size: 34448 bytes
Desc: llvm-lowercallto-refactor.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120515/5f232286/attachment.obj>

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