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?
MartinO
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
Also see [llvm-dev] SelectionDAGISel::Select's API considered harmful
for the motivation for the change.
Best,
Alex
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
"Martin J. O'Riordan" <martin.oriordan@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!
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