Page MenuHomePhabricator

[DAGCombiner] fold binops with constant into select-of-constants
ClosedPublic

Authored by spatel on Mar 1 2017, 8:35 AM.

Details

Summary

This is part of the ongoing attempt to improve select codegen for all targets and select canonicalization in IR (see D24480 for more background).
The transform is a subset of what is done in InstCombine's FoldOpIntoSelect().

I first noticed a regression in the x86 avx512-insert-extract.ll tests with a patch that hopes to convert more selects to basic math ops. This appears to be a general missing DAG transform though, so I added tests for all standard binops in rL296621 (PowerPC was chosen semi-randomly; it has scripted FileCheck support, but so do ARM and x86).

The debug output for "sel_constants_shl_constant" makes me think that this patch is not at fault for that mess. The shl fold happens as expected. PPC then changes the i8 select to an i32 select, but the return value is i64, so the extend gets folded in creating an i64 select. I noticed this problem in a lesser form in D30180; it's why there weren't more improvements for PPC with that patch.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Mar 1 2017, 8:35 AM
efriedma edited edge metadata.Mar 1 2017, 11:07 AM

The debug output for "sel_constants_shl_constant" makes me think that this patch is not at fault for that mess.

I would agree with that assessment; you get the same terrible lowering for "select i1 %cond, i8 -20, i8 115".


Some targets intentionally fold the other way in certain cases: pulling a math operation out of a select can make the immediates cheaper. For example, consider "int y; int *a(int x) { return x ? &y : &y+1; }" on ARM. That isn't directly impacted by this, though, because ARM uses target-specific nodes.

Does it make sense to handle SELECT_CC in addition to SELECT?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1788 ↗(On Diff #90182)

Could you assert here that NewCT and NewCF are actually constants?

test/CodeGen/X86/setcc.ll
54 ↗(On Diff #90182)

Hmm... we really want movl+adcl rather than movw+adcw. Maybe orthogonal to this patch.

spatel marked an inline comment as done.Mar 1 2017, 12:47 PM

The debug output for "sel_constants_shl_constant" makes me think that this patch is not at fault for that mess.

I would agree with that assessment; you get the same terrible lowering for "select i1 %cond, i8 -20, i8 115".

Right - I filed a similar example as:
https://bugs.llvm.org/show_bug.cgi?id=32105

Some targets intentionally fold the other way in certain cases: pulling a math operation out of a select can make the immediates cheaper. For example, consider "int y; int *a(int x) { return x ? &y : &y+1; }" on ARM. That isn't directly impacted by this, though, because ARM uses target-specific nodes.

Should I add a target hook or some other means to protect against that case? Or is it ok to assume this is universally good for the currently handled binop types?

Does it make sense to handle SELECT_CC in addition to SELECT?

I'm only seeing SELECT in the limited scope that I'm looking at so far. I'll add a TODO comment for now if that's ok.

spatel updated this revision to Diff 90220.Mar 1 2017, 12:50 PM

Patch updated:

  1. Add asserts for constant folds of the new select operands.
  2. Add vector tests (x86 for ease of testing) because there was no coverage for those.
  3. Add TODO comment about handling SELECT_CC.
efriedma accepted this revision.Mar 1 2017, 1:50 PM

LGTM.

Or is it ok to assume this is universally good for the currently handled binop types?

Okay for now; if someone eventually trips over it, we can deal with it then.

This revision is now accepted and ready to land.Mar 1 2017, 1:50 PM
This revision was automatically updated to reflect the committed changes.