[InstCombine] Add single use checks to SimplifyBSwap to ensure we are really saving instructions

[craig.topper]

Summary

Bswap isn't a simple operation so we need to make sure we are really removing a call to it before doing these simplifications.

For the case when both LHS and RHS are bswaps I've allowed it to be moved if either LHS or RHS has a single use since that at least allows us to move it later where it might find another bswap to combine with and it decreases the use count on the other side so maybe the other user can be optimized.

[spatel]

LGTM. See inline for an NFC pre- or post-commit cleanup.

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

78–81

It's independent of this patch, but this comment is too specific for this function because it doesn't mention the pattern with a constant operand. I'd make the comment more general and/or remove it. The function name could also mention 'bitwise op' since that is a requirement for any transform in this function.

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

assert(FCmpInst::FCMP_FALSE <= Pred && Pred <= FCmpInst::FCMP_TRUE &&
          "Unexpected FCmp predicate!");
   if (Pred == FCmpInst::FCMP_FALSE)
     return ConstantInt::get(CmpInst::makeCmpResultType(LHS->getType()), 0);
   if (Pred == FCmpInst::FCMP_TRUE)
     return ConstantInt::get(CmpInst::makeCmpResultType(LHS->getType()), 1);
   return Builder->CreateFCmp(Pred, LHS, RHS);
 }
 
 /// \brief Transform BITWISE_OP(BSWAP(A),BSWAP(B)) to BSWAP(BITWISE_OP(A, B))
 /// \param I Binary operator to transform.
 /// \return Pointer to node that must replace the original binary operator, or
 ///         null pointer if no transformation was made.
 Value *InstCombiner::SimplifyBSwap(BinaryOperator &I) {
   assert(I.isBitwiseLogicOp() && "Unexpected opcode for bswap simplifying");
 
-  // TODO We should probably check for single use of the bswap.
+  Value *OldLHS = I.getOperand(0);
+  Value *OldRHS = I.getOperand(1);
 
   Value *NewLHS;
-  if (!match(I.getOperand(0), m_BSwap(m_Value(NewLHS))))
+  if (!match(OldLHS, m_BSwap(m_Value(NewLHS))))
     return nullptr;
 
   Value *NewRHS;
   const APInt *C;
 
-  if (match(I.getOperand(1), m_BSwap(m_Value(NewRHS)))) {
+  if (match(OldRHS, m_BSwap(m_Value(NewRHS)))) {
     // OP( BSWAP(x), BSWAP(y) ) -> BSWAP( OP(x, y) )
+    if (!OldLHS->hasOneUse() && !OldRHS->hasOneUse())
+      return nullptr;
     // NewRHS initialized by the matcher.
-  } else if (match(I.getOperand(1), m_APInt(C))) {
+  } else if (match(OldRHS, m_APInt(C))) {
     // OP( BSWAP(x), CONSTANT ) -> BSWAP( OP(x, BSWAP(CONSTANT) ) )
+    if (!OldLHS->hasOneUse())
+      return nullptr;
     NewRHS = ConstantInt::get(I.getType(), C->byteSwap());
   } else
     return nullptr;
 
   Value *BinOp = Builder->CreateBinOp(I.getOpcode(), NewLHS, NewRHS);
   Function *F = Intrinsic::getDeclaration(I.getModule(), Intrinsic::bswap,
                                           I.getType());
   return Builder->CreateCall(F, BinOp);

test/Transforms/InstCombine/bswap-fold.ll

;
   %tmp2 = xor <2 x i32> %tmp1, <i32 100001, i32 100001>
   ret <2 x i32> %tmp2
 }
 
 define i64 @bs_and64_multiuse1(i64 %a, i64 %b) #0 {
 ; CHECK-LABEL: @bs_and64_multiuse1(
 ; CHECK-NEXT:    [[TMP1:%.*]] = tail call i64 @llvm.bswap.i64(i64 [[A:%.*]])
 ; CHECK-NEXT:    [[TMP2:%.*]] = tail call i64 @llvm.bswap.i64(i64 [[B:%.*]])
-; CHECK-NEXT:    [[TMP1:%.*]] = and i64 [[A]], [[B]]
-; CHECK-NEXT:    [[TMP2:%.*]] = call i64 @llvm.bswap.i64(i64 [[TMP1]])
-; CHECK-NEXT:    [[TMP4:%.*]] = mul i64 [[TMP2]], [[TMP1]]
+; CHECK-NEXT:    [[TMP3:%.*]] = and i64 [[TMP1]], [[TMP2]]
+; CHECK-NEXT:    [[TMP4:%.*]] = mul i64 [[TMP3]], [[TMP1]]
 ; CHECK-NEXT:    [[TMP5:%.*]] = mul i64 [[TMP4]], [[TMP2]]
 ; CHECK-NEXT:    ret i64 [[TMP5]]
 ;
   %tmp1 = tail call i64 @llvm.bswap.i64(i64 %a)
   %tmp2 = tail call i64 @llvm.bswap.i64(i64 %b)
   %tmp3 = and i64 %tmp1, %tmp2
   %tmp4 = mul i64 %tmp3, %tmp1 ; to increase use count of the bswaps
   %tmp5 = mul i64 %tmp4, %tmp2 ; to increase use count of the bswaps
;
   %tmp3 = and i64 %tmp1, %tmp2
   %tmp4 = mul i64 %tmp3, %tmp2 ; to increase use count of the bswaps
   ret i64 %tmp4
 }
 
 define i64 @bs_and64i_multiuse(i64 %a, i64 %b) #0 {
 ; CHECK-LABEL: @bs_and64i_multiuse(
 ; CHECK-NEXT:    [[TMP1:%.*]] = tail call i64 @llvm.bswap.i64(i64 [[A:%.*]])
-; CHECK-NEXT:    [[TMP1:%.*]] = and i64 [[A]], 129085117527228416
-; CHECK-NEXT:    [[TMP2:%.*]] = call i64 @llvm.bswap.i64(i64 [[TMP1]])
+; CHECK-NEXT:    [[TMP2:%.*]] = and i64 [[TMP1]], 1000000001
 ; CHECK-NEXT:    [[TMP3:%.*]] = mul i64 [[TMP2]], [[TMP1]]
 ; CHECK-NEXT:    ret i64 [[TMP3]]
 ;
   %tmp1 = tail call i64 @llvm.bswap.i64(i64 %a)
   %tmp2 = and i64 %tmp1, 1000000001
   %tmp3 = mul i64 %tmp2, %tmp1 ; to increase use count of the bswap
   ret i64 %tmp3
 }
 
 declare i16 @llvm.bswap.i16(i16)
 declare i32 @llvm.bswap.i32(i32)
 declare i64 @llvm.bswap.i64(i64)
 declare <2 x i32> @llvm.bswap.v2i32(<2 x i32>)