This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] add missing folds for scalar select of {-1,0,1}

Authored by spatel on Feb 20 2017, 2:32 PM.



I think all of the test changes here are wins or neutral, but anyone who knows AMDGPU, Hexagon, and NVPTX should take a look at those diffs. If it makes it easier, I can post the full before/after asm of the affected tests for those targets.

The motivation for filling out these select-of-constants cases goes back to D24480, where we discussed removing an IR fold from add(zext) --> select. And that goes back to:

The idea is that we should always canonicalize patterns like this to a select-of-constants in IR because that's the smallest IR and the best for value tracking. Note that we currently do the opposite in some cases (like the cases in *this* patch). Ie, the proposed folds in this patch already exist in InstCombine today:

As this patch shows, most targets generate better machine code for simple ext/add/not ops rather than a select of constants. So the follow-up steps to make this less of a patchwork of special-case folds and missing IR canonicalization:

  1. Have DAGCombiner convert any select of constants into ext/add/not ops.
  2. Have InstCombine canonicalize in the other direction (create more selects).

Diff Detail


Event Timeline

spatel created this revision.Feb 20 2017, 2:32 PM
jlebar accepted this revision.Feb 20 2017, 3:34 PM
jlebar added a subscriber: jlebar.

NVPTX test change is fine.

This revision is now accepted and ready to land.Feb 20 2017, 3:34 PM
kparzysz edited edge metadata.Feb 21 2017, 5:56 AM

The Hexagon changes are fine as well.

@tstellar @arsenm or anyone else with AMDGPU experience, do the test/CodeGen/AMDGPU/trunc.ll diffs look ok?

tstellar accepted this revision.Feb 23 2017, 10:14 AM
This revision was automatically updated to reflect the committed changes.