Page MenuHomePhabricator


Authored by dylanmckay on Oct 5 2016, 6:30 AM.



This pulls the AVR instruction selector in-tree.

Diff Detail


Event Timeline

dylanmckay updated this revision to Diff 73640.Oct 5 2016, 6:30 AM
dylanmckay retitled this revision from to [AVR] Add AVRISelDAGToDAG.cpp.
dylanmckay updated this object.
dylanmckay added reviewers: kparzysz, arsenm.
dylanmckay added a subscriber: japaric.
dylanmckay set the repository for this revision to rL LLVM.Oct 6 2016, 11:38 PM
dylanmckay added a subscriber: llvm-commits.
kparzysz requested changes to this revision.Oct 11 2016, 7:22 AM
kparzysz edited edge metadata.

Please use the recommended workflow for selection where applicable: selectXyz(n) functions replace n with selected code and return void. The use of selectImpl looks like a remnant of some old code.

112 ↗(On Diff #73640)

You can factor the isUInt out of this condition, e.g,:
if (isUInt<6>(RHSC) && (VT == MVT::i8 || VT == MVT::i16))

130 ↗(On Diff #73640)

I think it should be "== ISD::NON_EXTLOAD".

This revision now requires changes to proceed.Oct 11 2016, 7:22 AM
dylanmckay updated this revision to Diff 74319.Oct 11 2016, 9:39 PM
dylanmckay edited edge metadata.
dylanmckay marked 2 inline comments as done.

Rename selectImpl to trySelect and make it replace nodes and return boolean

A few minor fixups as suggested by Krzysztof.

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.

kparzysz added inline comments.Oct 12 2016, 8:06 AM
132 ↗(On Diff #74319)

All the "return 0" in this function should be "return nullptr".

dylanmckay updated this revision to Diff 74819.Oct 17 2016, 1:54 AM
dylanmckay marked an inline comment as done.
dylanmckay edited edge metadata.

Change the order we chose/replace/remove nodes during selection.

This brings it into line with more backends.

I chose to do the replacement/removal inside the individual node selection functions,
as this offers the most flexibility.

kparzysz accepted this revision.Oct 19 2016, 10:56 AM
kparzysz edited edge metadata.

The check for NON_EXTLOAD seems to be wrong again. LGTM otherwise.

129 ↗(On Diff #74819)

This has become != again.

This revision is now accepted and ready to land.Oct 19 2016, 10:56 AM

The check for NON_EXTLOAD seems to be wrong again. LGTM otherwise.

I've been looking into this deeper, and something is weird with this.

I'll commit this for now and look into it later.

This revision was automatically updated to reflect the committed changes.