Hello, Attached is a patch which significantly reworks how calls, incoming arguments, and outgoing return values are lowered. It's a major change, affecting all targets, so I'm looking for feedback on the approach. The goal of the patch is to eliminate a bunch of awkward code, eliminate some unnecessary differences between targets, and to facilitate future refactoring and feature work. This patch gets rid of ISD::CALL, ISD::FORMAL_ARGUMENTS, ISD::RET, and ISD::ISD::ARG_FLAGS, as well as the old LowerArguments and LowerCallTo hooks. To replace them, it adds three new TargetLowering hooks: LowerCall, LowerFormalArguments, and LowerReturn. These hooks provide targets with the same information as the special nodes, except in an immediately usable form instead of awkwardly encoded as SDNode operands. The patch also re-works a substantial portion of the target-independent tail-call code. The patch also includes changes for all in-tree targets to use the new hooks. Beyond dejagnu and some manual assembly output inspection, so far I've only tested this on x86 targets. I'm not in a hurry with this; I'm just looking for input at this point. Dan -------------- next part -------------- A non-text attachment was scrubbed... Name: calling-conv-lowering.patch Type: application/octet-stream Size: 296248 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090423/eec17ddf/attachment.obj>
Dan Gohman wrote:> Hello, > > Attached is a patch which significantly reworks how calls, incoming > arguments, and outgoing return values are lowered. It's a major change, > affecting all targets, so I'm looking for feedback on the approach. > > The goal of the patch is to eliminate a bunch of awkward code, > eliminate some unnecessary differences between targets, and to > facilitate future refactoring and feature work. > > This patch gets rid of ISD::CALL, ISD::FORMAL_ARGUMENTS, ISD::RET, and > ISD::ISD::ARG_FLAGS, as well as the old LowerArguments and LowerCallTo > hooks. To replace them, it adds three new TargetLowering hooks: > LowerCall, LowerFormalArguments, and LowerReturn. These hooks provide > targets with the same information as the special nodes, except in an > immediately usable form instead of awkwardly encoded as SDNode > operands. The patch also re-works a substantial portion of the > target-independent tail-call code. The patch also includes changes > for all in-tree targets to use the new hooks. > > Beyond dejagnu and some manual assembly output inspection, > so far I've only tested this on x86 targets. I'm not in a hurry > with this; I'm just looking for input at this point. > > DanHi Dan, I quickly ran my tests with your changes and a few of them breaks llc while a few other have incorrect assembly. I am attaching the .bc files here. The llc options I use --pre-RA-sched=list-burr and -regalloc=pbqp fun_in_expr3.bc (llc breaks) struct_args_5.bc (incorrect assemly, execution fails) char_char.bc (llc breaks) I haven't really examined what is the reason of failures. Will do so as soon as I get some time off from rest of the stuff. Let me know if you need any specific inputs on pic16 port. - Sanjiv> ------------------------------------------------------------------------ > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- A non-text attachment was scrubbed... Name: fun_in_expr3.bc Type: application/octet-stream Size: 604 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090424/dfae698f/attachment.obj> -------------- next part -------------- A non-text attachment was scrubbed... Name: struct_args_5.bc Type: application/octet-stream Size: 1172 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090424/dfae698f/attachment-0001.obj> -------------- next part -------------- A non-text attachment was scrubbed... Name: char_char.bc Type: application/octet-stream Size: 620 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090424/dfae698f/attachment-0002.obj>
Hi, Dan> The goal of the patch is to eliminate a bunch of awkward code, > eliminate some unnecessary differences between targets, and to > facilitate future refactoring and feature work.I quickly looked over the patch and it seems to be a significant cleanup of all really ugly lowering code! Maybe it will be possible to provide some dummy implementation of LowerFormalArguments / LowerReturn (pass everything on stack, etc), so one willing to add new backend won't need to implement such complex piece of code as a first step? -- With best regards, Anton Korobeynikov Faculty of Mathematics and Mechanics, Saint Petersburg State University
On Apr 23, 2009, at 8:09 PM, Dan Gohman wrote:> Attached is a patch which significantly reworks how calls, incoming > arguments, and outgoing return values are lowered. It's a major > change, > affecting all targets, so I'm looking for feedback on the approach. > > The goal of the patch is to eliminate a bunch of awkward code, > eliminate some unnecessary differences between targets, and to > facilitate future refactoring and feature work.Nice!> This patch gets rid of ISD::CALL, ISD::FORMAL_ARGUMENTS, ISD::RET, and > ISD::ISD::ARG_FLAGS, as well as the old LowerArguments and LowerCallTo > hooks. To replace them, it adds three new TargetLowering hooks: > LowerCall, LowerFormalArguments, and LowerReturn. These hooks provide > targets with the same information as the special nodes, except in an > immediately usable form instead of awkwardly encoded as SDNode > operands. The patch also re-works a substantial portion of the > target-independent tail-call code. The patch also includes changes > for all in-tree targets to use the new hooks.This is a great improvement! Most of my comments are general design thoughts, not specific to this one patch. I understand that this is one step in the right direction. Do clients end up lowering the return information before they lower the argument information? If they do, it should be relatively straight-forward to support "spilling to the stack" return values that can't fit in physregs. It would be nice to eventually support arbitrary return values in the code generator. Doing this would basically just require return lowering to push an extra iPtr argument. Looking at the patch, it seems odd that ISD::LIBCALL still exists. Is the "CSE of libcalls" optimization really important in practice? If so, perhaps this should be done as an explicit dag combine optimization: if two clumps of flagged together nodes are identical and have the same inputs, replace one clump with the other. This would be a nice general solution instead of something specific to calls. Unrelated, but it would also be nice to make INLINEASM nodes have their own SDNode subclass someday instead of a bunch of magically indexed operands. Similar to Nate's recent shuffle change. InputArg/OutputArg should probably contain an ArgFlagsTy instead of inherit from it. I like the various -'s of random call lowering stuff from the .td files, this is great! -Chris
On Apr 24, 2009, at 10:27 AM, Sanjiv Gupta wrote:> Dan Gohman wrote: >> Hello, >> >> Attached is a patch which significantly reworks how calls, incoming >> arguments, and outgoing return values are lowered. It's a major >> change, >> affecting all targets, so I'm looking for feedback on the approach. >> >> The goal of the patch is to eliminate a bunch of awkward code, >> eliminate some unnecessary differences between targets, and to >> facilitate future refactoring and feature work. >> >> This patch gets rid of ISD::CALL, ISD::FORMAL_ARGUMENTS, ISD::RET, >> and >> ISD::ISD::ARG_FLAGS, as well as the old LowerArguments and >> LowerCallTo >> hooks. To replace them, it adds three new TargetLowering hooks: >> LowerCall, LowerFormalArguments, and LowerReturn. These hooks provide >> targets with the same information as the special nodes, except in an >> immediately usable form instead of awkwardly encoded as SDNode >> operands. The patch also re-works a substantial portion of the >> target-independent tail-call code. The patch also includes changes >> for all in-tree targets to use the new hooks. >> >> Beyond dejagnu and some manual assembly output inspection, >> so far I've only tested this on x86 targets. I'm not in a hurry >> with this; I'm just looking for input at this point. >> >> Dan > Hi Dan, > I quickly ran my tests with your changes and a few of them breaks > llc while a few other have incorrect assembly. I am attaching > the .bc files here. The llc options I use --pre-RA-sched=list-burr > and -regalloc=pbqp > > fun_in_expr3.bc (llc breaks) > struct_args_5.bc (incorrect assemly, execution fails) > char_char.bc (llc breaks)Thanks for the testing and the testcases. PIC16 is certainly the most complicated of the bunch :-}. I have fixes for each of these, and with them I've verified that a patched llc is emitting the same output as an unpatched llc, with one exception which I believe is ok. I'll post details and an updated patch soon. Dan
On Apr 24, 2009, at 11:49 AM, Anton Korobeynikov wrote:> Hi, Dan > >> The goal of the patch is to eliminate a bunch of awkward code, >> eliminate some unnecessary differences between targets, and to >> facilitate future refactoring and feature work. > I quickly looked over the patch and it seems to be a significant > cleanup of all really ugly lowering code! > > Maybe it will be possible to provide some dummy implementation of > LowerFormalArguments / LowerReturn (pass everything on stack, etc), so > one willing to add new backend won't need to implement such complex > piece of code as a first step?My goal is for the CallingConvLowering code to eventually take over most, if not all, of the work for these hooks. Then, someone writing a new target could just add a simple CallingConv that just does CCAssignToStack for everything as a first step. I don't know when that might happen, but with this patch it's a little easier to think about. Dan
On Apr 25, 2009, at 1:35 PM, Chris Lattner wrote:> > Do clients end up lowering the return information before they lower > the argument information? If they do, it should be relatively > straight-forward to support "spilling to the stack" return values that > can't fit in physregs. It would be nice to eventually support > arbitrary return values in the code generator. Doing this would > basically just require return lowering to push an extra iPtr argument.There are details, but yes, PR2660 will be easier to approach with this patch :-).> > > Looking at the patch, it seems odd that ISD::LIBCALL still exists. Is > the "CSE of libcalls" optimization really important in practice? If > so, perhaps this should be done as an explicit dag combine > optimization: if two clumps of flagged together nodes are identical > and have the same inputs, replace one clump with the other. This > would be a nice general solution instead of something specific to > calls.It comes up in test/CodeGen/ARM/cse-libcalls.ll, at least. However, that may be due to the particular way SELECT_CC is legalized. I'm going to re-evaluate this.> > > Unrelated, but it would also be nice to make INLINEASM nodes have > their own SDNode subclass someday instead of a bunch of magically > indexed operands. Similar to Nate's recent shuffle change.Patches welcome :-).> > > InputArg/OutputArg should probably contain an ArgFlagsTy instead of > inherit from it.I think you're right. I'll make this change. Thanks, Dan
On Thursday 23 April 2009 22:09, Dan Gohman wrote:> Hello, > > Attached is a patch which significantly reworks how calls, incoming > arguments, and outgoing return values are lowered. It's a major change, > affecting all targets, so I'm looking for feedback on the approach.I don't have specific feedback on this patch but I do have feedback about how we go about making these kinds of changing. This patch changes the LLVM API. We should have a process for deprecating obsolete interfaces before removing them entirely. It's a significant maintenance headache to pull down a new release and fix all of the API issues along with tracking down new bugs introduced. I completely agree with the goals of this patch. However, I would ask that we deprecate the existing interfaces for 2.6 and then remove them in 2.7. We should add the new interfaces under a different name until 2.7 at which point they will become default. LLVM is getting significant 3rd-party use and we should figure out how to act in a new way to better support our users. -Dave
>> Attached is a patch which significantly reworks how calls, incoming >> arguments, and outgoing return values are lowered. It's a major change, >> affecting all targets, so I'm looking for feedback on the approach. > > I don't have specific feedback on this patch but I do have feedback about how > we go about making these kinds of changing. > > This patch changes the LLVM API. We should have a process for deprecating > obsolete interfaces before removing them entirely. It's a significant > maintenance headache to pull down a new release and fix all of the API > issues along with tracking down new bugs introduced. > > I completely agree with the goals of this patch. However, I would ask that we > deprecate the existing interfaces for 2.6 and then remove them in 2.7. We > should add the new interfaces under a different name until 2.7 at which point > they will become default.I don't really see how this is any different. Instead of changing the API in 2.6, it just gets postponed to 2.7. So with 2.7 you have to upgrade your API and track down new bugs introduced by 2.7 (hopefully not very many). The majority of the people are not going to make the API changes until they are forced to. Its just low on the priority list. We've seen this from many people who don't upgrade because they don't want to make the API changes. -Tanya> > LLVM is getting significant 3rd-party use and we should figure out how to act > in a new way to better support our users. > > -Dave > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
On Apr 29, 2009, at 8:39 AM, David Greene wrote:> > This patch changes the LLVM API. We should have a process for > deprecating > obsolete interfaces before removing them entirely. It's a significant > maintenance headache to pull down a new release and fix all of the API > issues along with tracking down new bugs introduced.No, we make no attempt at being API compatible across revs of LLVM. This is a great way to encourage people to contribute their code to the project. If they want to live with their code out of tree, then they have to deal with API breakage. -Chris