This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] convert and-of-seteq to bitwise logic+seteq (PR32401)
ClosedPublic

Authored by spatel on Mar 29 2017, 4:03 PM.

Details

Summary

This is a generic combine to reduce icmp logic as discussed in:
https://bugs.llvm.org/show_bug.cgi?id=32401

I think these are all wins for targets in the affected tests, but a target could reverse this if it is not a win (or we can add a target hook to selectively disable this). A follow-up would allow us to canonicalize IR to the icmp form in instcombine.

I had initially enabled this for vectors too, but there were several regressions because of the scalar-limited constant matching in the combines above this one (and likely we're accidentally excluding vectors from folds that would reduce to those forms).

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Mar 29 2017, 4:03 PM
efriedma added inline comments.Mar 29 2017, 4:30 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3251 ↗(On Diff #93419)

If you're going to do this, it would be nice to at least handle the inverse case: or(setne, setne). Not sure what other related cases are worth transforming.

test/CodeGen/ARM/and-setcc.ll
27 ↗(On Diff #93419)

This isn't the shortest possible sequence; for example, gcc generates:

cmp     r0, r1
cmpeq   r2, r3
moveq   r0, #1
movne   r0, #0
bx      lr

Probably not worth worrying about, though.

spatel added inline comments.Mar 30 2017, 6:40 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3251 ↗(On Diff #93419)

Sure - I was planning to look at the related patterns in "visitORLike" as well as the vector cases as follow-ups. Would you prefer that I pull it together in one patch, or I can add another FIXME comment?

efriedma edited edge metadata.Mar 30 2017, 11:51 AM

Someone is inevitably going to request a target hook for this; the reason this transform looks good is that all the targets you have tests for require at least 3 instructions to lower seteq. If you have a one-instruction seteq operation, the seteq+seteq+and sequence is cheaper. (Not sure what targets have this for scalars, but a lot of targets have this for vectors, e.g. x86 pcmpeqd.)

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3251 ↗(On Diff #93419)

It doesn't need to be in the same patch, as long as you don't forget about it.

spatel updated this revision to Diff 93970.Apr 3 2017, 4:34 PM

Patch updated:

  1. Add a target hook to selectively enable this transform.
  2. Enable the hook for scalars on x86, PPC, and ARM.
  3. Add the 'or' sibling fold (I did several NFC clean-up commits last week, so this could be added simply here).
spatel added inline comments.Apr 4 2017, 6:54 AM
test/CodeGen/PowerPC/setcc-logic.ll
454–456 ↗(On Diff #93970)

I think this sequence is better than crand+isel, but another possibility:
https://bugs.llvm.org/show_bug.cgi?id=32520

efriedma accepted this revision.Apr 4 2017, 11:36 AM

I'm not really familiar enough with PPC to comment on those changes. Otherwise LGTM.

This revision is now accepted and ready to land.Apr 4 2017, 11:36 AM
nemanjai accepted this revision.Apr 5 2017, 1:21 AM

I think the PPC changes look good.

test/CodeGen/PowerPC/setcc-logic.ll
454–456 ↗(On Diff #93970)

This is in the pipeline:
https://reviews.llvm.org/D31240

This revision was automatically updated to reflect the committed changes.