Martin J. O'Riordan via llvm-dev
2016-Jun-28 17:55 UTC
[llvm-dev] Question about changes to 'SelectionDAGISel.h'
Thanks Ahmed and also Alex for your replies. This is more or less what I was realising, but it is a great confidence booster to know that it is the correct way also. I can replace all of my various 'Select*' specialisations with version that use 'ReplaceNode/SelectCode' and return 'void', but what about the places where I currently call 'Select(N)' directly? Should I substitute 'SelectCode(N)' instead? I will examine the X86 implementation as you recommend and hope to glean some knowledge from that. All the best, MartinO -----Original Message----- From: Ahmed Bougacha [mailto:ahmed.bougacha at gmail.com] Sent: 28 June 2016 17:02 To: Martin.ORiordan at movidius.com Cc: LLVM Developers <llvm-dev at lists.llvm.org>; Justin Bogner <mail at justinbogner.com> Subject: Re: [llvm-dev] Question about changes to 'SelectionDAGISel.h' On Tue, Jun 28, 2016 at 8:53 AM, Martin J. O'Riordan via llvm-dev <llvm-dev at lists.llvm.org> wrote:> It occurred to me that instead of the various breakout ‘Select*’ > functions returning the ‘SDNode*’ result, maybe I should be calling: > > > > ReplaceNode(N, newValue); > > return; > > or: > > SelectCode(N); > > return; > > > > Perhaps?Yes, I think the core difference is that Select() - not its caller - now does the replacement, so there's nothing to return. This is actually mentioned in the release notes (kudos to Justin!): SelectionDAGISel::Select now returns void. Out of tree targets will need to be updated to replace the argument node and remove any dead nodes in cases where they currently return an SDNode * from this interface. http://llvm.org/docs/ReleaseNotes.html You can look at the changes to the various in-tree targets between r268693 and r270454 (e.g., r269144 for x86). -Ahmed> > > MartinO > > > > From: Martin J. O'Riordan [mailto:martin.oriordan at movidius.com] > Sent: 28 June 2016 16:49 > To: 'LLVM Developers' <llvm-dev at lists.llvm.org> > Subject: Question about changes to 'SelectionDAGISel.h' > > > > Although I would like to track the LLVM head revisions regularly, > unfortunately I only get the opportunity every couple of months or > even every 6 month with a full release. > > > > This time I am updating from revision #262824 (8th March) so more than > 3 months have elapsed. For the most part I have completed the changes > I need to make, but I’m stuck on one change that is more significant > than it appears. > > > > In the March #262824 revision ‘SelectionDAGISel’ declared the function > ‘Select’ as: > > > > SDNode *Select(SDNode *N) = 0; > > > > But now it is: > > > > void Select(SDNode *N) = 0; > > > > The problem is that we have a number of places where we construct DAGs > using the returned ‘SDNode*’ from explicit calls to ‘Select’ and I am > wondering what is the best design approach for me to fix this? This > is old code, so the architectural changes that lead to the return type > for ‘Select’ being changed have probably been long coming, but I was > unaware of it. Should I be using ‘SelectCode’ instead for example? > Or is there a good reference target I should examine to see how they adapted? > > > > Thanks, > > > > MartinO > > > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
Justin Bogner via llvm-dev
2016-Jun-28 21:04 UTC
[llvm-dev] Question about changes to 'SelectionDAGISel.h'
"Martin J. O'Riordan" <martin.oriordan at movidius.com> writes:> Thanks Ahmed and also Alex for your replies. > > This is more or less what I was realising, but it is a great > confidence booster to know that it is the correct way also. I can > replace all of my various 'Select*' specialisations with version that > use 'ReplaceNode/SelectCode' and return 'void', but what about the > places where I currently call 'Select(N)' directly? Should I > substitute 'SelectCode(N)' instead?Maybe, but be aware that `N` won't necessarily be a valid reference anymore after calling SelectCode - it may have been replaced. If you use the value that Select(N) used to return, try to refactor so you don't have to. You can probably do this by pulling pieces of Select into more specific helpers. You can find lots of examples of how to do the void Select transition in the version control history - look for commits with "SelectImpl" in the title. That said, the basic playbook I used for conversions went something like this: 1. Avoid leaving dangling nodes around in your Select function - places that currently call ReplaceUses() and then return nullptr often do that. There are examples of this in r269256. 2. Similarly, some places might replace all uses of a node with a new one, then return the new node. Just remove the dead node instead. There was some of this in r269358. 3. Anywhere else where Select returns a node, update it to call ReplaceNode instead (or ReplaceUses + RemoveDeadNode). All of the "Implement Select instead of SelectImpl" commits do some of this. 4. Where a utility function can return null when Select should fall back to another selector, rename that to try*, have it call ReplaceNode, and return a bool for success. 5. Where something calls SelectNodeTo, just return afterwards (SelectNodeTo does the appropriate replacement). Hope that helps!> I will examine the X86 implementation as you recommend and hope to > glean some knowledge from that. > > All the best, > > MartinO > > -----Original Message----- > From: Ahmed Bougacha [mailto:ahmed.bougacha at gmail.com] > Sent: 28 June 2016 17:02 > To: Martin.ORiordan at movidius.com > Cc: LLVM Developers <llvm-dev at lists.llvm.org>; Justin Bogner > <mail at justinbogner.com> > Subject: Re: [llvm-dev] Question about changes to 'SelectionDAGISel.h' > > On Tue, Jun 28, 2016 at 8:53 AM, Martin J. O'Riordan via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> It occurred to me that instead of the various breakout ‘Select*’ >> functions returning the ‘SDNode*’ result, maybe I should be calling: >> >> >> >> ReplaceNode(N, newValue); >> >> return; >> >> or: >> >> SelectCode(N); >> >> return; >> >> >> >> Perhaps? > > Yes, I think the core difference is that Select() - not its caller - now does the replacement, so there's nothing to return. > > This is actually mentioned in the release notes (kudos to Justin!): > > SelectionDAGISel::Select now returns void. Out of tree targets will need to be updated to replace the argument node and remove any dead nodes in cases where they currently return an SDNode * from this interface. > > http://llvm.org/docs/ReleaseNotes.html > > You can look at the changes to the various in-tree targets between > r268693 and r270454 (e.g., r269144 for x86). > > -Ahmed > >> >> >> MartinO >> >> >> >> From: Martin J. O'Riordan [mailto:martin.oriordan at movidius.com] >> Sent: 28 June 2016 16:49 >> To: 'LLVM Developers' <llvm-dev at lists.llvm.org> >> Subject: Question about changes to 'SelectionDAGISel.h' >> >> >> >> Although I would like to track the LLVM head revisions regularly, >> unfortunately I only get the opportunity every couple of months or >> even every 6 month with a full release. >> >> >> >> This time I am updating from revision #262824 (8th March) so more than >> 3 months have elapsed. For the most part I have completed the changes >> I need to make, but I’m stuck on one change that is more significant >> than it appears. >> >> >> >> In the March #262824 revision ‘SelectionDAGISel’ declared the function >> ‘Select’ as: >> >> >> >> SDNode *Select(SDNode *N) = 0; >> >> >> >> But now it is: >> >> >> >> void Select(SDNode *N) = 0; >> >> >> >> The problem is that we have a number of places where we construct DAGs >> using the returned ‘SDNode*’ from explicit calls to ‘Select’ and I am >> wondering what is the best design approach for me to fix this? This >> is old code, so the architectural changes that lead to the return type >> for ‘Select’ being changed have probably been long coming, but I was >> unaware of it. Should I be using ‘SelectCode’ instead for example? >> Or is there a good reference target I should examine to see how they adapted? >> >> >> >> Thanks, >> >> >> >> MartinO >> >> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>
Martin J. O'Riordan via llvm-dev
2016-Jun-29 12:50 UTC
[llvm-dev] Question about changes to 'SelectionDAGISel.h'
Hi Justin, Thanks very much for taking the time to write such a helpful response, this is really useful to me. All the best, MartinO -----Original Message----- From: Justin Bogner [mailto:justin at justinbogner.com] On Behalf Of Justin Bogner Sent: 28 June 2016 22:05 To: Martin J. O'Riordan Cc: 'Ahmed Bougacha'; 'LLVM Developers' Subject: Re: [llvm-dev] Question about changes to 'SelectionDAGISel.h' "Martin J. O'Riordan" <martin.oriordan at movidius.com> writes:> Thanks Ahmed and also Alex for your replies. > > This is more or less what I was realising, but it is a great > confidence booster to know that it is the correct way also. I can > replace all of my various 'Select*' specialisations with version that > use 'ReplaceNode/SelectCode' and return 'void', but what about the > places where I currently call 'Select(N)' directly? Should I > substitute 'SelectCode(N)' instead?Maybe, but be aware that `N` won't necessarily be a valid reference anymore after calling SelectCode - it may have been replaced. If you use the value that Select(N) used to return, try to refactor so you don't have to. You can probably do this by pulling pieces of Select into more specific helpers. You can find lots of examples of how to do the void Select transition in the version control history - look for commits with "SelectImpl" in the title. That said, the basic playbook I used for conversions went something like this: 1. Avoid leaving dangling nodes around in your Select function - places that currently call ReplaceUses() and then return nullptr often do that. There are examples of this in r269256. 2. Similarly, some places might replace all uses of a node with a new one, then return the new node. Just remove the dead node instead. There was some of this in r269358. 3. Anywhere else where Select returns a node, update it to call ReplaceNode instead (or ReplaceUses + RemoveDeadNode). All of the "Implement Select instead of SelectImpl" commits do some of this. 4. Where a utility function can return null when Select should fall back to another selector, rename that to try*, have it call ReplaceNode, and return a bool for success. 5. Where something calls SelectNodeTo, just return afterwards (SelectNodeTo does the appropriate replacement). Hope that helps!> I will examine the X86 implementation as you recommend and hope to > glean some knowledge from that. > > All the best, > > MartinO > > -----Original Message----- > From: Ahmed Bougacha [mailto:ahmed.bougacha at gmail.com] > Sent: 28 June 2016 17:02 > To: Martin.ORiordan at movidius.com > Cc: LLVM Developers <llvm-dev at lists.llvm.org>; Justin Bogner > <mail at justinbogner.com> > Subject: Re: [llvm-dev] Question about changes to 'SelectionDAGISel.h' > > On Tue, Jun 28, 2016 at 8:53 AM, Martin J. O'Riordan via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> It occurred to me that instead of the various breakout ‘Select*’ >> functions returning the ‘SDNode*’ result, maybe I should be calling: >> >> >> >> ReplaceNode(N, newValue); >> >> return; >> >> or: >> >> SelectCode(N); >> >> return; >> >> >> >> Perhaps? > > Yes, I think the core difference is that Select() - not its caller - now does the replacement, so there's nothing to return. > > This is actually mentioned in the release notes (kudos to Justin!): > > SelectionDAGISel::Select now returns void. Out of tree targets will need to be updated to replace the argument node and remove any dead nodes in cases where they currently return an SDNode * from this interface. > > http://llvm.org/docs/ReleaseNotes.html > > You can look at the changes to the various in-tree targets between > r268693 and r270454 (e.g., r269144 for x86). > > -Ahmed > >> >> >> MartinO >> >> >> >> From: Martin J. O'Riordan [mailto:martin.oriordan at movidius.com] >> Sent: 28 June 2016 16:49 >> To: 'LLVM Developers' <llvm-dev at lists.llvm.org> >> Subject: Question about changes to 'SelectionDAGISel.h' >> >> >> >> Although I would like to track the LLVM head revisions regularly, >> unfortunately I only get the opportunity every couple of months or >> even every 6 month with a full release. >> >> >> >> This time I am updating from revision #262824 (8th March) so more >> than >> 3 months have elapsed. For the most part I have completed the >> changes I need to make, but I’m stuck on one change that is more >> significant than it appears. >> >> >> >> In the March #262824 revision ‘SelectionDAGISel’ declared the >> function ‘Select’ as: >> >> >> >> SDNode *Select(SDNode *N) = 0; >> >> >> >> But now it is: >> >> >> >> void Select(SDNode *N) = 0; >> >> >> >> The problem is that we have a number of places where we construct >> DAGs using the returned ‘SDNode*’ from explicit calls to ‘Select’ and >> I am wondering what is the best design approach for me to fix this? >> This is old code, so the architectural changes that lead to the >> return type for ‘Select’ being changed have probably been long >> coming, but I was unaware of it. Should I be using ‘SelectCode’ instead for example? >> Or is there a good reference target I should examine to see how they adapted? >> >> >> >> Thanks, >> >> >> >> MartinO >> >> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>