This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] replace duplicated peekThroughBitcast helper functions; NFCI
ClosedPublic

Authored by spatel on Sep 19 2018, 5:19 PM.

Details

Summary

x86 had 2 versions of peekThroughBitcast. DAGCombiner had 1. Plus, it had a 1-off implementation for the one-use variant.
The new code mimics the IR version where we have an optional param if the one-use constraint is requested. No functional change intended.

I'm proposing to put this next to isBitwiseNot() because I am planning to use it in there. Another option is next to the helpers in the ISD namespace (eg, ISD::isConstantSplatVector()). But if there's no good reason for those to be there, I'd prefer to pull everything over here if possible.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Sep 19 2018, 5:19 PM

Would we be better off keeping the separate peekThroughOneUseBitcasts helper for clarity? Still putting them both in SelectionDAG

Would we be better off keeping the separate peekThroughOneUseBitcasts helper for clarity? Still putting them both in SelectionDAG

Either way is ok with me. I took this option since it's what we do in IR, but spelling it out in the name does enforce better readability. Anyone else have a preference?

spatel updated this revision to Diff 166283.Sep 20 2018, 6:45 AM

Patch updated:
Use separate functions to differentiate the one-use constraint. This is effectively transferring the x86 code with the same names, so it's a smaller patch.

Implementation detail: the x86 code was checking for 'V.getNode()', but I'm not sure how that could be false, so I left that out of the new code.

Unless there is a need to conditionally use the one-use variant,
two separate functions seem cleaner. But no strong opinion here.

RKSimon accepted this revision.Sep 20 2018, 8:47 AM

LGTM - thanks

This revision is now accepted and ready to land.Sep 20 2018, 8:47 AM
This revision was automatically updated to reflect the committed changes.