[PATCH] D33050: [InstCombine] remove fold that swaps xor/or with constants

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
Herald added a subscriber: mcrosier.

I can't see the value in this fold:
// (X ^ C1) | C2 --> (X | C2) ^ (C1&~C2)
as an optimization or canonicalization, so I'm proposing to remove it.

Some examples of where this might fire:

  define i8 @not_or(i8 %x) {
    %xor = xor i8 %x, -1
    %or = or i8 %xor, 7
    ret i8 %or
  }
  define i8 @xor_or(i8 %x) {
    %xor = xor i8 %x, 32
    %or = or i8 %xor, 7
    ret i8 %or
  }
  define i8 @xor_or2(i8 %x) {
    %xor = xor i8 %x, 33
    %or = or i8 %xor, 7
    ret i8 %or
  }

Regardless of whether we remove this fold, we get:

  define i8 @not_or(i8 %x) {
    %xor = or i8 %x, 7
    %or = xor i8 %xor, -8
    ret i8 %or
  }  
  define i8 @xor_or(i8 %x) {
    %xor = or i8 %x, 7
    %or = xor i8 %xor, 32
    ret i8 %or
  }
  define i8 @xor_or2(i8 %x) {
    %xor = or i8 %x, 7
    %or = xor i8 %xor, 32
    ret i8 %or
  }

There are no test changes because our current demanded-bits handling for xor constants will always remove set bits of the xor constant, and then we'll activate the fold a few lines later under the comment:

  // (X^C)|Y -> (X|Y)^C iff Y&C == 0

The larger motivation for removing this code is that it could interfere with the fix for PR32706:
https://bugs.llvm.org/show_bug.cgi?id=32706

Ie, we're not checking if the 'xor' is actually a 'not', so we could reverse a 'not' optimization and cause an infinite loop by altering an 'xor X, -1'. We can restore this fold after the demanded-bits change is restored if we can find a reason it helps?


https://reviews.llvm.org/D33050

Files:
  lib/Transforms/InstCombine/InstCombineAndOrXor.cpp


Index: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -1986,18 +1986,6 @@
   if (Value *V = SimplifyBSwap(I))
     return replaceInstUsesWith(I, V);
 
-  if (ConstantInt *RHS = dyn_cast<ConstantInt>(Op1)) {
-    ConstantInt *C1 = nullptr; Value *X = nullptr;
-    // (X ^ C1) | C2 --> (X | C2) ^ (C1&~C2)
-    if (match(Op0, m_Xor(m_Value(X), m_ConstantInt(C1))) &&
-        Op0->hasOneUse()) {
-      Value *Or = Builder->CreateOr(X, RHS);
-      Or->takeName(Op0);
-      return BinaryOperator::CreateXor(Or,
-                            Builder->getInt(C1->getValue() & ~RHS->getValue()));
-    }
-  }
-
   if (isa<Constant>(Op1))
     if (Instruction *FoldedLogic = foldOpWithConstantIntoOperand(I))
       return FoldedLogic;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D33050.98477.patch
Type: text/x-patch
Size: 916 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170510/b53e7ef6/attachment.bin>