[ARM] Thumb2: ConstantMaterializationCost

[SjoerdMeijer]

Summary

Constants can also be materialised using the negated value and a MVN, and this
case seem to have been missed for Thumb2. To check the constant materialisation
costs, we now call getT2SOImmVal twice, once for the original constant and then
also for its negated value, and this function checks if the constant can both
be splatted or rotated.

This was revealed by a test that optimises for minsize: instead of a LDR
literal pool load and having a literal pool entry, just a MVN with an immediate is
smaller (and also faster). This gives a small code size improvement for all our
apps in our code corpus, only 2 test cases have a tiny, insignificant
regression.

[efriedma]

Maybe put the tests into test/CodeGen/ARM/subtarget-no-movt.ll? We have some similar tests there from https://reviews.llvm.org/D23090 .

lib/Target/ARM/ARMISelDAGToDAG.cpp

453–454

I don't think the change from getT2SOImmValSplatVal to getT2SOImmVal makes much practical difference; isThumbImmShiftedVal covers the same cases. But I guess it's more clear, at least.

Maybe fix the MOV/MVN/MOVW comments so they're each on the same line as the corresponding check?

test/CodeGen/ARM/shifter_operand.ll

262

You can put the attributes inline here (instead of "#0", just write "optsize minsize").

[SjoerdMeijer]

lib/Target/ARM/ARMISelDAGToDAG.cpp

453–454

Forgot to mention that, but indeed, I found there was no practical difference, but thought it is clearer.

test/CodeGen/ARM/shifter_operand.ll

262

Thanks, didn't know that!

[SjoerdMeijer]

Addressed comments.

[efriedma]

Maybe put the tests into test/CodeGen/ARM/subtarget-no-movt.ll? We have some similar tests there from https://reviews.llvm.org/D23090 .

[SjoerdMeijer]

Maybe put the tests into test/CodeGen/ARM/subtarget-no-movt.ll? We have some similar tests there from https://reviews.llvm.org/D23090 .

Ah, sorry, somehow I missed that comment.

• The test cases have been moved to test/CodeGen/ARM/subtarget-no-movt.ll, and
• while I was at it, cleaned up the duplicated mess that was this file (and it also included a typo USE-MOVTT-O0 resulting in FileCheck not checking that line, one day I would really like to change FileCheck so that it errors for these cases...)

[efriedma]

LGTM with one nit.

lib/Target/ARM/ARMISelDAGToDAG.cpp

454–456

The "+" here is misleading (especially given other nearby comments); either replace it with a "/", or put the checks on separate lines.

[SjoerdMeijer]

lib/Target/ARM/ARMISelDAGToDAG.cpp

454–456

Thanks, will do before committing.

lib/Target/ARM/ARMISelDAGToDAG.cpp

bool ARMDAGToDAGISel::isShifterOpProfitable(const SDValue &Shift,
   // R << 2 is free.
   return ShOpcVal == ARM_AM::lsl &&
          (ShAmt == 2 || (Subtarget->isSwift() && ShAmt == 1));
 }
 
 unsigned ARMDAGToDAGISel::ConstantMaterializationCost(unsigned Val) const {
   if (Subtarget->isThumb()) {
     if (Val <= 255) return 1;                               // MOV
     if (Subtarget->hasV6T2Ops() &&
-        (Val <= 0xffff || ARM_AM::getT2SOImmValSplatVal(Val) != -1))
-      return 1; // MOVW
+        (Val <= 0xffff || ARM_AM::getT2SOImmVal(Val) != -1 || // MOV + MOVW
+         ARM_AM::getT2SOImmVal(~Val) != -1))                  // MVN
+      return 1;
     if (Val <= 510) return 2;                               // MOV + ADDi8
     if (~Val <= 255) return 2;                              // MOV + MVN
     if (ARM_AM::isThumbImmShiftedVal(Val)) return 2;        // MOV + LSL
   } else {
     if (ARM_AM::getSOImmVal(Val) != -1) return 1;           // MOV
     if (ARM_AM::getSOImmVal(~Val) != -1) return 1;          // MVN
     if (Subtarget->hasV6T2Ops() && Val <= 0xffff) return 1; // MOVW
     if (ARM_AM::isSOImmTwoPartVal(Val)) return 2;           // two instrs

test/CodeGen/ARM/shifter_operand.ll

; CHECK-THUMB: movt [[CONST]], #1
   %ptr = inttoptr i32 %sum to i32*
   %loaded = load i32, i32* %ptr
 
   %ret.tmp = insertvalue { i32, i32 } undef, i32 %sum, 0
   %ret = insertvalue { i32, i32 } %ret.tmp, i32 %loaded, 1
 
   ret { i32, i32 } %ret
 }
+
+declare dso_local i32 @eat_const(i32)
+
+define hidden i32 @no_litpool() minsize optsize {
+; CHECK-LABEL:  no_litpool:
+; CHECK:        mov{{.*}} r0, #65536
+; CHECK:        mov{{.*}} r0, #-134217728
+; CHECK:        mvn r0, #-134217728
+entry:
+  %call0 = tail call i32 @eat_const(i32 65536)
+  %call1 = tail call i32 @eat_const(i32 -134217728)
+  %call2 = tail call i32 @eat_const(i32 134217727)
+  ret i32 %call2
+}
+
+define hidden i32 @litpool() minsize optsize {
+; CHECK-LABEL:  litpool:
+; CHECK:        ldr r0, {{.*}}LCPI{{.*}}
+; CHECK-NEXT:   b {{.*}}eat_const
+entry:
+  %call1 = tail call i32 @eat_const(i32 8388601)
+  ret i32 %call1
+}