[PATCH] D25278: [AVR] Add AVRISelDAGToDAG.cpp

[Krzysztof Parzyszek via llvm-commits]

kparzysz added a comment.

This still isn't quite right.

Going top-down: the Select(N) function generates a new machine node for N, replaces uses of N with the NewN and removes the now dead node N.  This completes the selection of N and there is no need to return any value, so it returns void. If a specific node N does not need special handling, Select would call SelectCode(N), which then completes the selection of N, including the replacement and the cleanup. While SelectCode returns SDNode*, it's always nullptr.

The body of Select can be broken up into individual functions that handle specific types of nodes, which is what you did and which is good. At this point you need to decide if each of these functions will do a complete selection (i.e. by calling RemoveDeadNode or SelectCode when necessary), or if they simply generate a new node when they can and only do ReplaceUses (leaving RemoveDeadNode/SelectCode to the main Select function), or take yet another approach where they only generate the new node and leave the replacement/dead node removal, or calling SelectCode to the main Select. The first option allows each of such function to use a wider variety of replacement functions (e.g. ReplaceNode instead of just ReplaceUses, or SelectNodeTo).

Your code doesn't seem to be consistent in what the individual functions do and return. For example select<FrameIndex> calls SelectNodeTo (which removes dead node), while select<LOAD> may not even do any replacement (if it calls selectIndexedLoad).

After a node has been removed, it should not really be accessed. Currently SelectionDAG does not actually delete them, but that is not something that should be relied on.  The existing DAGToDAG code for many targets is messy, but most of that code predates the changes to how selection works. The "Select" function used to return the newly selected node, but that has changed and now it returns void.


https://reviews.llvm.org/D25278