[ARM] Mark -1 as cheap in xor's for thumb1

[dmgreen]

Summary

We can always convert xor %a, -1 into MVN, even in thumb 1 where the -1
would not otherwise be considered a cheap constant. This prevents the
-1's from being pulled out into constants and potentially hoisted.

The loop_2 tests are the only ones that change here.

[spatel]

This is the same motivating case as D42951 - abandon that patch?

I haven't looked at ConstantHoisting much, but this seems like a good solution to the problem. I wonder if we could generalize this for all targets by checking if a 'not' op is available.

Might want to commit the tests first with auto-generated output using utils/update_llc_test_checks.py, so we just see the improvement here.

There's a FIXME note about global isel for the div-by-constant case, so this problem also disappears once we have that? Maybe the whole thing disappears, so it's not worth repeating the comment.

[dmgreen]

In D43451#1012076, @spatel wrote:

This is the same motivating case as D42951 - abandon that patch?

Yep. This is part one out of two or three. I'm not sure yet, depends on how part 2 looks. I don't think it will solve the register pressure part of the problem (not entirely at least), but it should always be a net gain when it comes up.

I haven't looked at ConstantHoisting much, but this seems like a good solution to the problem. I wonder if we could generalize this for all targets by checking if a 'not' op is available.

Hmm. I didn't even consider other targets. I think the problem is getIntImmCost above (without an opcode) returning 2 for thumb1 targets. Everything else (Thumb2/Arm) is going to treat -1 as a cheap constant anyway. For other targets, I believe the default is to return Free for all constants.

Might want to commit the tests first with auto-generated output using utils/update_llc_test_checks.py, so we just see the improvement here.

Sure, sounds sensible. Like I said it's just loop_2 that's updated here.

There's a FIXME note about global isel for the div-by-constant case, so this problem also disappears once we have that? Maybe the whole thing disappears, so it's not worth repeating the comment.

Yeah I think so. I presume global ISel will have a much better time looking across basic blocks and a lot of codegen prepare/constant merge wont be needed any more. I think that comment applies to all costs in this function.

[dmgreen]

I realised I could just show the updates here without having to commit the changes first. So that's what this is.

I've not committed the left hand side yet, let me know if I should anyway. Otherwise this should show the differences.

[spatel]

LGTM.

[efriedma]

llvm/trunk/lib/Target/ARM/ARMTargetTransformInfo.cpp

131 ↗

(On Diff #135038)

getSExtValue() will crash if Imm doesn't fit into an int64_t. Maybe use isAllOnesValue()?

[dmgreen]

llvm/trunk/lib/Target/ARM/ARMTargetTransformInfo.cpp

131 ↗

(On Diff #135038)

Ah. Where it's not -1. Of course. Sorry about that, thanks for the catch.

https://reviews.llvm.org/D43549

lib/Target/ARM/ARMTargetTransformInfo.cpp

if (Opcode == Instruction::ICmp && Imm.isNegative() &&
     if (ST->isThumb2() && NegImm < 1<<12)
       // icmp X, #-C -> cmn X, #C
       return 0;
     if (ST->isThumb() && NegImm < 1<<8)
       // icmp X, #-C -> adds X, #C
       return 0;
   }
 
+  // xor a, -1 can always be folded to MVN
+  if (Opcode == Instruction::Xor) {
+    int64_t NegImm = Imm.getSExtValue();
+    if (NegImm == -1)
+      return 0;
+  }
+
   return getIntImmCost(Imm, Ty);
 }
 
 int ARMTTIImpl::getCastInstrCost(unsigned Opcode, Type *Dst, Type *Src,
                                  const Instruction *I) {
   int ISD = TLI->InstructionOpcodeToISD(Opcode);
   assert(ISD && "Invalid opcode");
 

test/CodeGen/Thumb/mvn.ll

for.body:
   br i1 %exitcond, label %for.cond.cleanup, label %for.body
 
 for.cond.cleanup:
   ret void
 }
 
 define void @loop8_2(i8* %a, i8* %b) {
 ; CHECK-LABEL: loop8_2:
-; CHECK:         .save {r4, r5, r7, lr}
-; CHECK-NEXT:    push {r4, r5, r7, lr}
+; CHECK:         .save {r4, lr}
+; CHECK-NEXT:    push {r4, lr}
 ; CHECK-NEXT:    movs r2, #0
-; CHECK-NEXT:    movs r3, #255
 ; CHECK-NEXT:  .LBB3_1:
-; CHECK-NEXT:    ldrb r4, [r1, r2]
-; CHECK-NEXT:    ldrb r5, [r0, r2]
-; CHECK-NEXT:    eors r5, r3
-; CHECK-NEXT:    strb r5, [r0, r2]
-; CHECK-NEXT:    eors r4, r3
-; CHECK-NEXT:    strb r4, [r1, r2]
+; CHECK-NEXT:    ldrb r3, [r1, r2]
+; CHECK-NEXT:    ldrb r4, [r0, r2]
+; CHECK-NEXT:    mvns r4, r4
+; CHECK-NEXT:    strb r4, [r0, r2]
+; CHECK-NEXT:    mvns r3, r3
+; CHECK-NEXT:    strb r3, [r1, r2]
 ; CHECK-NEXT:    adds r2, r2, #1
 ; CHECK-NEXT:    cmp r2, #10
 ; CHECK-NEXT:    bne .LBB3_1
-; CHECK-NEXT:    pop {r4, r5, r7, pc}
+; CHECK-NEXT:    pop {r4, pc}
 entry:
   br label %for.body
 
 for.body:
   %i = phi i32 [ 0, %entry ], [ %inc, %for.body ]
   %arrayidx = getelementptr inbounds i8, i8* %a, i32 %i
   %arrayidx2 = getelementptr inbounds i8, i8* %b, i32 %i
   %x = load i8, i8* %arrayidx
for.body:
   br i1 %exitcond, label %for.cond.cleanup, label %for.body
 
 for.cond.cleanup:
   ret void
 }
 
 define void @loop32_2(i32* %a, i32* %b) {
 ; CHECK-LABEL: loop32_2:
-; CHECK:         .save {r4, r5, r7, lr}
-; CHECK-NEXT:    push {r4, r5, r7, lr}
+; CHECK:         .save {r4, lr}
+; CHECK-NEXT:    push {r4, lr}
 ; CHECK-NEXT:    movs r2, #0
-; CHECK-NEXT:    mvns r3, r2
 ; CHECK-NEXT:  .LBB7_1:
-; CHECK-NEXT:    ldr r4, [r1, r2]
-; CHECK-NEXT:    ldr r5, [r0, r2]
-; CHECK-NEXT:    eors r5, r3
-; CHECK-NEXT:    str r5, [r0, r2]
-; CHECK-NEXT:    eors r4, r3
-; CHECK-NEXT:    str r4, [r1, r2]
+; CHECK-NEXT:    ldr r3, [r1, r2]
+; CHECK-NEXT:    ldr r4, [r0, r2]
+; CHECK-NEXT:    mvns r4, r4
+; CHECK-NEXT:    str r4, [r0, r2]
+; CHECK-NEXT:    mvns r3, r3
+; CHECK-NEXT:    str r3, [r1, r2]
 ; CHECK-NEXT:    adds r2, r2, #4
 ; CHECK-NEXT:    cmp r2, #40
 ; CHECK-NEXT:    bne .LBB7_1
-; CHECK-NEXT:    pop {r4, r5, r7, pc}
+; CHECK-NEXT:    pop {r4, pc}
 entry:
   br label %for.body
 
 for.body:
   %i = phi i32 [ 0, %entry ], [ %inc, %for.body ]
   %arrayidx = getelementptr inbounds i32, i32* %a, i32 %i
   %arrayidx2 = getelementptr inbounds i32, i32* %b, i32 %i
   %x = load i32, i32* %arrayidx