[ARM] Remove some code from PerformCMOVCombine

[samparker]

Summary

There is a small piece of combine code that is no longer covered by the existing tests and which also looks incorrect. The provided test case was performing movne r0, r0 for the return value.

[efriedma]

The code in question is hit pretty frequently; see http://lab.llvm.org:8080/coverage/coverage-reports/all/coverage/Users/buildslave/jenkins/workspace/clang-stage2-coverage-R/llvm/lib/Target/ARM/ARMISelLowering.cpp.html#L12550 . Maybe we don't have any tests which check this particular aspect of the output, though.

The provided test case was performing movne r0, r0 for the return value.

That's obviously not ideal, but deleting the transform isn't really improving the situation. (https://rise4fun.com/Alive/slIT ).

[samparker]

I'm happy to move it somewhere general, but where would be suitable, InstCombine?

[efriedma]

If you want to specifically detect a "no-op" select, like your testcase, instcombine is probably appropriate. (I think we already have that combine, but maybe it doesn't look through zero-extensions.)

The redundant mov is really a property of ARM specifically, though: unlike other architectures, like AArch64, a conditional mov is a two-address instruction on ARM, so we want to reuse an existing register if possible. And we don't really represent the "conditional-move" aspect until after legalization, so I'm not sure where we would move this code to, despite the FIXME.

[samparker]

Ok, thanks for the clarification. I'll have a look in instcombine.

lib/Target/ARM/ARMISelLowering.cpp

 
   // BFI is only available on V6T2+.
   if (!Subtarget->isThumb1Only() && Subtarget->hasV6T2Ops()) {
     SDValue R = PerformCMOVToBFICombine(N, DAG);
     if (R)
       return R;
   }
 
-  // Simplify
-  //   mov     r1, r0
-  //   cmp     r1, x
-  //   mov     r0, y
-  //   moveq   r0, x
-  // to
-  //   cmp     r0, x
-  //   movne   r0, y
-  //
-  //   mov     r1, r0
-  //   cmp     r1, x
-  //   mov     r0, x
-  //   movne   r0, y
-  // to
-  //   cmp     r0, x
-  //   movne   r0, y
-  /// FIXME: Turn this into a target neutral optimization?
   SDValue Res;
-  if (CC == ARMCC::NE && FalseVal == RHS && FalseVal != LHS) {
-    Res = DAG.getNode(ARMISD::CMOV, dl, VT, LHS, TrueVal, ARMcc,
-                      N->getOperand(3), Cmp);
-  } else if (CC == ARMCC::EQ && TrueVal == RHS) {
-    SDValue ARMcc;
-    SDValue NewCmp = getARMCmp(LHS, RHS, ISD::SETNE, ARMcc, DAG, dl);
-    Res = DAG.getNode(ARMISD::CMOV, dl, VT, LHS, FalseVal, ARMcc,
-                      N->getOperand(3), NewCmp);
-  }
-
   // (cmov F T ne CPSR (cmpz (cmov 0 1 CC CPSR Cmp) 0))
   // -> (cmov F T CC CPSR Cmp)
   if (CC == ARMCC::NE && LHS.getOpcode() == ARMISD::CMOV && LHS->hasOneUse()) {
     auto *LHS0C = dyn_cast<ConstantSDNode>(LHS->getOperand(0));
     auto *LHS1C = dyn_cast<ConstantSDNode>(LHS->getOperand(1));
     auto *RHSC = dyn_cast<ConstantSDNode>(RHS);
     if ((LHS0C && LHS0C->getZExtValue() == 0) &&
         (LHS1C && LHS1C->getZExtValue() == 1) &&

test/CodeGen/ARM/select.ll

 ; CHECK-LABEL: f12:
 define float @f12(i32 %a, i32 %b) nounwind uwtable readnone ssp {
 ; CHECK-NOT: floatunsisf
   %1 = icmp eq i32 %a, %b
   %2 = uitofp i1 %1 to float
   ret float %2
 }
 
+; CHECK-LABEL: f13
+; CHECK: ldr
+; CHECK: ldr
+; CHECK: cmp
+; CHECK: moveq r0, r1
+; CHECK: mov pc
+define i16 @f13(i8* %arg0, i16* %arg1) {
+entry:
+  %0 = load i8, i8* %arg0
+  %conv = zext i8 %0 to i16
+  %conv1 = zext i8 %0 to i32
+  %1 = load i16, i16* %arg1
+  %conv2 = zext i16 %1 to i32
+  %cmp = icmp eq i32 %conv1, %conv2
+  %res = select i1 %cmp, i16 %1, i16 %conv
+  ret i16 %res
+}