This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] (Y + ~X) + 1 --> Y - X fold (PR42459)
ClosedPublic

Authored by lebedev.ri on Jul 1 2019, 5:41 AM.

Details

Summary

To be noted, this pattern is not unhandled by instcombine per-se,
it is somehow does end up being folded when one runs opt -O3,
but not if it's just -instcombine. Regardless, that fold is
indirect, depends on some other folds, and is thus blind
when there are extra uses.

This does address the regression being exposed in previous patch D63992.

https://godbolt.org/z/7DGltU
https://rise4fun.com/Alive/EPO0

Fixes PR42459

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri updated this revision to Diff 207292.Jul 1 2019, 6:53 AM

So apparently we were already handling the (A + 1) + ~B case,
but not this case with +1 being the outer instruction..
Move the fold to the original fold.

spatel added inline comments.Jul 1 2019, 6:59 AM
lib/Transforms/InstCombine/InstCombineAddSub.cpp
1203–1209

Here's the sibling matcher. The -reassociation pass tries to create this form (move constant operands up in a chain of reassociative ops).

lebedev.ri marked 2 inline comments as done.Jul 1 2019, 7:03 AM
lebedev.ri added inline comments.
lib/Transforms/InstCombine/InstCombineAddSub.cpp
1203–1209

Yep, found it once i understood that the fold *was* being done by instcombine after all (after adding commutative tests), and moved the fold here.
Though i don't understand the -print-after-all output, -reassociation does not run there..

spatel accepted this revision.Jul 1 2019, 7:33 AM

LGTM

lib/Transforms/InstCombine/InstCombineAddSub.cpp
1203–1209

The descriptive text for -reassociation is "Reassociate expressions", so that might be obscuring your search, but I see the expected diff:

*** IR Dump After Simplify the CFG ***
; Function Attrs: norecurse nounwind readnone
define i32 @t0(i32 %x, i32 %y) local_unnamed_addr #0 {
  %o0 = xor i32 %x, -1
  %o1 = add i32 %o0, %y
  %o2 = add i32 %o1, 1
  ret i32 %o2
}
*** IR Dump After Reassociate expressions ***
; Function Attrs: norecurse nounwind readnone
define i32 @t0(i32 %x, i32 %y) local_unnamed_addr #0 {
  %o0 = xor i32 %x, -1
  %o1 = add i32 %o0, 1
  %o2 = add i32 %o1, %y
  ret i32 %o2
}
This revision is now accepted and ready to land.Jul 1 2019, 7:33 AM
lebedev.ri edited the summary of this revision. (Show Details)Jul 1 2019, 7:37 AM
lebedev.ri marked 3 inline comments as done.

Thank you for the review!

lib/Transforms/InstCombine/InstCombineAddSub.cpp
1203–1209

Oh yeah, thank you!
I was looking for something else, and kind-of did not notice *that* change.

This revision was automatically updated to reflect the committed changes.