This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] extend(ifpositive(X)) -> shift-right (not X)
ClosedPublic

Authored by spatel on Jul 5 2018, 7:24 AM.

Details

Summary

This is almost the same as an existing IR canonicalization in instcombine, so I'm assuming this is a good early generic DAG combine too.

The motivation comes from reduced bit-hacking for select-of-constants in IR after rL331486. We want to restore that functionality in the DAG as noted in the commit comments for that change and the llvm-dev discussion here:
http://lists.llvm.org/pipermail/llvm-dev/2018-July/124433.html

The PPC and AArch tests show that those targets are already doing something similar. x86 will be neutral in the minimal case and generally better when this pattern is extended with other ops as shown in the signbit-shift.ll tests.

Note the asymmetry: I didn't include the (extend (ifneg X)) transform because it already exists in SimplifySelectCC(), and that is verified in the later unchanged tests in the signbit-shift.ll files. Without the 'not' op, the general transform to use a shift is always a win because that's a single instruction.

I don't know if the XCore test diff is a regression, but given that we canonicalize IR to this form, I'd think that could be changed independently if needed.

Alive proofs:
https://rise4fun.com/Alive/ysli

Name: if pos, get -1
  %c = icmp sgt i16 %x, -1
  %r = sext i1 %c to i16
=>
  %n = xor i16 %x, -1
  %r = ashr i16 %n, 15

Name: if pos, get 1
  %c = icmp sgt i16 %x, -1
  %r = zext i1 %c to i16
=>
  %n = xor i16 %x, -1
  %r = lshr i16 %n, 15

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jul 5 2018, 7:24 AM
spatel edited the summary of this revision. (Show Details)Jul 5 2018, 7:29 AM
craig.topper added inline comments.Jul 5 2018, 10:59 AM
test/CodeGen/X86/signbit-shift.ll
74 ↗(On Diff #154229)

Do you have a longer term plan here?

This looks like it could be something like:

shrl $31, %edi
add $41, %edi
mov %edi, %eax
spatel added inline comments.Jul 5 2018, 11:12 AM
test/CodeGen/X86/signbit-shift.ll
74 ↗(On Diff #154229)

Right. I think the same problem is visible for the 'ifneg' variant below. I have another patch drafted that would have included the trailing add/sub op, but I saw this minimal pattern, so decided we should squash this first. If this works, then I can follow-up by matching this new pattern that includes a shift.

spatel added inline comments.Jul 5 2018, 11:14 AM
test/CodeGen/X86/signbit-shift.ll
74 ↗(On Diff #154229)

I think these are the ideal results for the select tests (for x86 at least, but hopefully all targets):
https://rise4fun.com/Alive/Avq

lebedev.ri accepted this revision.Jul 6 2018, 12:10 AM

Code looks good, but i can't comment on the test changes.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8006 ↗(On Diff #154229)

We are assuming that we won't get setge <>, 0, because some dagcombine already canonicalizes that?

This revision is now accepted and ready to land.Jul 6 2018, 12:10 AM
spatel added inline comments.Jul 6 2018, 6:36 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8006 ↗(On Diff #154229)

Correct - that transform is done both in instcombine and TargetLowering::SimplifySetCC():

// Canonicalize GE/LE comparisons to use GT/LT comparisons.

I'll add a comment to make that clear.

This revision was automatically updated to reflect the committed changes.