This is an archive of the discontinued LLVM Phabricator instance.

Delete more dead code in SelectionDAG (NFC)
ClosedPublic

Authored by vsk on Sep 9 2016, 6:07 PM.

Details

Summary

I tested this with "ninja check-llvm" on a Release build with all architectures
enabled, and on a Debug build (for just x86).

Found with llvm-cov.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk updated this revision to Diff 70936.Sep 9 2016, 6:07 PM
vsk retitled this revision from to Delete more dead code in SelectionDAG (NFC).
vsk updated this object.
vsk added reviewers: bogner, RKSimon.
vsk added a subscriber: llvm-commits.
RKSimon edited edge metadata.Sep 13 2016, 6:36 AM

The SelectNodeTo / getMachineNode helpers can probably go as they're very easy to re-add if we ever get a use case.

include/llvm/CodeGen/SelectionDAG.h
561 ↗(On Diff #70936)

This should stay while TargetIndexSDNode exists.

However, TargetIndexSDNode doesn't appear to be used so maybe start a conversion on llvm-dev about removing it and if successful then propose a separate patch?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
656 ↗(On Diff #70936)

Do you know the history behind this and DanglingDebugInfo::getSDNodeOrder()?

vsk added a comment.Sep 13 2016, 1:02 PM

Thanks for your notes.

include/llvm/CodeGen/SelectionDAG.h
561 ↗(On Diff #70936)

It looks like this code has out of tree users (see r250717), so I won't mess with it.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
656 ↗(On Diff #70936)

No, I don't. Is this something we're better off keeping?

mehdi_amini added inline comments.Sep 13 2016, 2:03 PM
include/llvm/CodeGen/SelectionDAG.h
561 ↗(On Diff #70936)

If there are out-of-tree users that care about having this API upstream, they should provide coverage testing for it, otherwise this can live in the downstream codebase...

As per Mehdi's suggestion - please ask on the llvm-dev list for somebody downstream to take ownership of TargetIndexSDNode/getTargetIndex and improve testing. Otherwise it shouldn't be left to rot in trunk (but that should be a separate patch).

The rest of the patch looks alright.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
656 ↗(On Diff #70936)

No, I think it can go.

vsk added a comment.Oct 6 2016, 10:23 AM

It looks like this patch is OK to land, minus changes related to SelectionDAG:: getTargetIndex. I filed http://llvm.org/PR30625 about adding test coverage for that function. I'll commit this later today if there are no more issues.

RKSimon accepted this revision.Oct 6 2016, 11:41 AM
RKSimon edited edge metadata.
In D24435#563675, @vsk wrote:

It looks like this patch is OK to land, minus changes related to SelectionDAG:: getTargetIndex. I filed http://llvm.org/PR30625 about adding test coverage for that function. I'll commit this later today if there are no more issues.

Agreed. LGTM

This revision is now accepted and ready to land.Oct 6 2016, 11:41 AM
This revision was automatically updated to reflect the committed changes.