Page MenuHomePhabricator

[InstCombine] Simplify and merge FoldOrWithConstants/FoldXorWithConstants
ClosedPublic

Authored by craig.topper on Aug 7 2017, 8:18 PM.

Details

Summary

The functions seem overly complicated. The body of this function is rechecking for an And operation to find the constant, but we already know we were looking at two Ands ORed together and the pieces are in variables. So just pass the correct pieces and check for two Constants.

Also since the two functions are almost identical, just pass in the opcode for the one difference so they can be combined.

Next step is to use m_APInt instead of ConstantInt.

Event Timeline

craig.topper created this revision.Aug 7 2017, 8:18 PM
zzheng added a subscriber: zzheng.Aug 8 2017, 11:41 AM
spatel added inline comments.Aug 13 2017, 10:51 AM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1813–1814

Dropped the null check on CI2? This will crash:

define i32 @D36439(i32 %a, i32 %b, i32 %c) {
  %tmp = or i32 %b, %a
  %tmp1 = and i32 %tmp, 1
  %tmp2 = and i32 %b, %c
  %tmp3 = or i32 %tmp1, %tmp2
  ret i32 %tmp3
}

Add null check for second ConstantInt.

Remove a change that accidentally crept into my last diff.

spatel added inline comments.Aug 13 2017, 3:05 PM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1955–1956

It looks functionally correct now, but I'm finding it hard to follow the matchers because we do half of it down here and then finish up in the helper. It also doesn't help that the comment formula variables don't line up with the code ('A' in the formula above is 'V1' in the code?). Can we just match the constants down here and get rid of the helper? Or move these initial matchers into the helper?

Remove the helper and move the code into an earlier block that already checked for constants.

spatel accepted this revision.Aug 13 2017, 4:27 PM

LGTM - thanks!

This revision is now accepted and ready to land.Aug 13 2017, 4:27 PM
This revision was automatically updated to reflect the committed changes.