Page MenuHomePhabricator

[InstCombine] canShiftBinOpWithConstantRHS(): drop bogus(?) signbit check
ClosedPublic

Authored by lebedev.ri on May 15 2019, 3:57 AM.

Details

Summary

In D61918 i was looking at dropping it in DAGCombiner visitShiftByConstant(),
but as @craig.topper pointed out, it was copied from here.

That check claims that the transform is illegal otherwise.
That isn't true:

  1. For ISD::ADD, we only process ISD::SHL outer shift => sign bit does not matter https://rise4fun.com/Alive/K4A
  2. For ISD::AND, there is no restriction on constants: https://rise4fun.com/Alive/Wy3
  3. For ISD::OR, there is no restriction on constants: https://rise4fun.com/Alive/GOH
  4. For ISD::XOR, there is no restriction on constants: https://rise4fun.com/Alive/ml6

So, why is it there then?
As far as i can tell, it dates all the way back to original check-in rL7793.
I think we should just drop it.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri edited the summary of this revision. (Show Details)May 15 2019, 4:31 AM

I don't see a reason for the constant limitation, but either way, we should have some minimal tests to verify (and change assuming this patch is committed):

define i32 @or_ashr(i32 %x) {
  %b = or i32 %x, 2147483648 ; 0x80000000
  %r = ashr i32 %b, 8
  ret i32 %r
}
define i32 @xor_ashr(i32 %x) {
  %b = xor i32 %x, 2147483648 ; 0x80000000
  %r = ashr i32 %b, 8
  ret i32 %r
}
; and with ashr is unreachable because it always get converted to lshr?

Also, add the related patterns with a 'select'?

lib/Transforms/InstCombine/InstCombineShifts.cpp
319 ↗(On Diff #199576)

Remove unreachable 'break'.

lebedev.ri marked an inline comment as done.

Added proper full test coverage.

I have manually checked these changed tests in alive, and they are all good.

spatel accepted this revision.May 16 2019, 6:48 AM

LGTM

This revision is now accepted and ready to land.May 16 2019, 6:48 AM
This revision was automatically updated to reflect the committed changes.