This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Remove foldSelectICmpAndOr. Instead call foldSelectICmpAnd as part of foldSelectIntoOp
Changes PlannedPublic

Authored by craig.topper on Aug 29 2017, 1:57 PM.

Details

Summary

foldSelectICmpAndOr handled turning (select (icmp eq (and X, C1), 0), Y, (or Y, C2)) into (or (shl (and X, C1), C3), Y)

But there was no reason reason the binary operator we're folding should be limited to Or. It should also work for xor, add, sub, etc.

Conceptually the code was a combination of foldSelectICmpAnd which takes care of turning bit test compares used to select constants into logic. And foldSelectIntoOp which tries to create a select on the input to a binary operator but stops if it would create a select of constants that isn't selecting between 0 and 1/-1.

So this patch teaches foldSelectIntoOp that if it would create a select of constants, but the condition is a bit test we can still handle it.

I've disable this if the binop is a shift because that seemed a little weirder.

I've added test for xor here but I can add more for other operators.

This also broke all the one use protections that existed in foldSelectICmpAndOr. This patch is already pretty large. So I can either fix them in foldSelectICmpAnd ahead of this or do it after this.

I can split up some of the refactorings and function reorderings out if we want.

Diff Detail

Event Timeline

craig.topper created this revision.

Forgot to delete the removed code.

spatel edited edge metadata.Sep 4 2017, 12:21 PM

I vote to split this up:

  1. Can you make the APInt changes for getSelectFoldableConstant() and isSelect01() as an NFC preliminary commit?
  2. Could also do the move of foldSelectICmpAnd() as a prelim to shrink this down to just the core diff.
  3. Add all of the new tests ahead of this.
  4. Fix the hasOneUse() deficiency first to avoid regressions.

Re: the pattern with a shift - add a blurb to the comments about *why* it's disabled?

craig.topper planned changes to this revision.Dec 14 2017, 9:47 AM