SelectionDAGISel::Select's API considered harmful

TLDR: Heads up for out of tree backends - you're going to need to update
your *DAGToDAGISel::Select method to unconditionally replace nodes
directly instead of returning the desired replacement.

So I'm working on fixing the undefined behaviour I described in
llvm.org/PR26808. As part of this, we need to stop looking into deleted
SDNodes to check if they were, in fact, deleted. A big place that we do
this is in SelectionDAGISel::DoInstructionSelection, where we can find
this helpful comment that came with the commit that added the check for
DELETED_NODE:

SelectionDAGISel.cpp says:

// FIXME: This is pretty gross. 'Select' should be changed to not return
// anything at all and this code should be nuked with a tactical strike.

I'm just gonna go ahead and take this advice.

I'll phase this in a couple of steps:

1. Rename Select to SelectImpl in all targets, and implement "virtual
   void Select(SDNode *)" in SelectionDAGISel. I'll move the current
   sketchy behaviour into this version of Select.

2. Update backends one at a time to implement "void Select(SDNode *)"
   instead of SelectImpl.

3. Make SelectionDAGISel::Select pure virtual and remove SelectImpl
   entirely.

If you have an out of tree backend and you merge from trunk, I recommend
updating in between steps 1 and 3 to avoid breakage after 3 happens.

Update: All in tree backends now implement `void Select`. I'll be
removing the SelectImpl path on Monday.

Justin Bogner <mail@justinbogner.com> writes:

Can you put something in the release notes when this happens?

Thanks,
Hans

Hans Wennborg <hans@chromium.org> writes:

Can you put something in the release notes when this happens?

I already updated the release notes in r268693, when I added the void
Select option in the first place :slight_smile:

Thanks,
Hans

Update: All in tree backends now implement `void Select`. I'll be
removing the SelectImpl path on Monday.

This is done in r270454.

Perfect, thanks!