[ARM] Don't use the Machine Scheduler for cortex-m at minsize

[dmgreen]

Summary

The new cortex-m schedule in D54142 helps performance, but can increase the amount of high-registers used. This, on average, ends up increasing the codesize by a fair amount (because less instructions are converted from T2 to T1). On cortex-m at -Oz, where we are quite size-paranoid, it is better to use the existing DAG scheduler with the RegPressure scheduling preference (at least until the issues around T2 vs T1 instructions can be improved).

I have also made sure that the Sched::RegPressure dag scheduler is always chosen for MinSize.

The test shows one case where we increase the number of registers used.

[samparker]

LGTM.

[fhahn]

This looks like an interesting case! I think there are 2 things going wrong (see input to the scheduler below)

1. The scheduler does not know it would be beneficial to keep things in the lower registers (the instructions mostly use the GPR/GPRNOPC classes). We could tweak the RegPressureSetLimits for Oz.
2. We add nodes that reduce register pressure to the available queue too late, because currently we only move from pending to available when we reached the ReadyCycle for in-order CPUs. This means we end up in a situation in which we have to schedule a node that further increases reg pressure.

While the patch is a quick fix, I think we should properly address those issues. Would you mind creating a bug report?

bb.0.entry:
  liveins: $r0, $r1
  %1:rgpr = COPY $r1
  %0:gpr = COPY $r0
  %2:rgpr = t2LSRri %1:rgpr, 8, 14, $noreg, $noreg
  %3:gprnopc = t2LDRi12 %0:gpr, 16, 14, $noreg :: (load 4 from %ir.msg_out)
  %4:rgpr = t2LDRHi12 %0:gpr, 20, 14, $noreg :: (load 2 from %ir.options_out_len, align 4)
  %5:rgpr = t2ADDri %4:rgpr, 1, 14, $noreg, $noreg
  t2STRHi12 %5:rgpr, %0:gpr, 20, 14, $noreg :: (store 2 into %ir.options_out_len, align 4)
  %6:gprnopc = t2ADDrr %3:gprnopc, %4:rgpr, 14, $noreg, $noreg
  t2STRBi12 %2:rgpr, %6:gprnopc, 240, 14, $noreg :: (store 1 into %ir.arrayidx)
  %7:gprnopc = t2LDRi12 %0:gpr, 16, 14, $noreg :: (load 4 from %ir.msg_out)
  %8:rgpr = t2LDRHi12 %0:gpr, 20, 14, $noreg :: (load 2 from %ir.options_out_len, align 4)
  %9:rgpr = t2ADDri %8:rgpr, 1, 14, $noreg, $noreg
  t2STRHi12 %9:rgpr, %0:gpr, 20, 14, $noreg :: (store 2 into %ir.options_out_len, align 4)
  %10:gprnopc = t2ADDrr %7:gprnopc, %8:rgpr, 14, $noreg, $noreg
  t2STRBi12 %1:rgpr, %10:gprnopc, 240, 14, $noreg :: (store 1 into %ir.arrayidx10)
  tBX_RET 14, $noreg

[SjoerdMeijer]

I agree with this:

While the patch is a quick fix, I think we should properly address those issues. Would you mind creating a bug report?

This is a workaround for suboptimal decision making elsewhere. Teaching the scheduler new tricks and heuristics is usually not low-hanging fruit. So, since this workaround is very minimal and clean, creating a bug report looks like an excellent way forward to me.

[fhahn]

LGTM, it would be great to have Cortex-M4 use the MachineScheduler in the non -Oz cases. Lets keep track of the issues (I expect more to surface) and go from there.

[SjoerdMeijer]

Yep, cheers, allocation of the high registers has been bothering us for a while now, and will indeed probably resurface sooner rather than later.

[dmgreen]

Sorry guys, I was distracted by some other work.

Florian, Nice one looking into the issues. I had tried some quick experiments around adjusting the priority of tryPressure in tryCandidate, but didn't get it to do much. I think because as you said the limits were a lot higher enough to kick in. My further ideas were more in the direction of adjusting the register allocation hints to pick higher registers less for instructions that can be shrunk into T1 instructions. Or (re-)adjusting the schedule taking into account things like CPSR live ranges, again for which instructions can and can't be shrank.

I've created a bug report in PR41884. Liek I tried to put into the comment, hopefully this is something that we don't need for long.

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

ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM,
   if (Subtarget->hasV6Ops())
     setTargetDAGCombine(ISD::SRL);
   if (Subtarget->isThumb1Only())
     setTargetDAGCombine(ISD::SHL);
 
   setStackPointerRegisterToSaveRestore(ARM::SP);
 
   if (Subtarget->useSoftFloat() || Subtarget->isThumb1Only() ||
-      !Subtarget->hasVFP2())
+      !Subtarget->hasVFP2() || Subtarget->hasMinSize())
     setSchedulingPreference(Sched::RegPressure);
   else
     setSchedulingPreference(Sched::Hybrid);
 
   //// temporary - rewrite interface to use type
   MaxStoresPerMemset = 8;
   MaxStoresPerMemsetOptSize = 4;
   MaxStoresPerMemcpy = 4; // For @llvm.memcpy -> sequence of stores

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

return isTargetELF() && TM.isPositionIndependent() &&
          !TM.shouldAssumeDSOLocal(*GV->getParent(), GV);
 }
 
 unsigned ARMSubtarget::getMispredictionPenalty() const {
   return SchedModel.MispredictPenalty;
 }
 
 bool ARMSubtarget::enableMachineScheduler() const {
+  // The MachineScheduler can increase register usage, so we use more high
+  // registers and end up with more T2 instructions that cannot be converted to
+  // T1 instructions. At least until we do better at converting to thumb1
+  // instructions, on cortex-m at Oz where we are size-paranoid, don't use the
+  // Machine scheduler, relying on the DAG register pressure scheduler instead.
+  if (isMClass() && hasMinSize())
+    return false;
   // Enable the MachineScheduler before register allocation for subtargets
   // with the use-misched feature.
   return useMachineScheduler();
 }
 
 // This overrides the PostRAScheduler bit in the SchedModel for any CPU.
 bool ARMSubtarget::enablePostRAScheduler() const {
   if (disablePostRAScheduler())

llvm/trunk/test/CodeGen/Thumb2/m4-sched-regs.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc %s -o - | FileCheck %s
 
 target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
 target triple = "thumbv7em-arm-none-eabi"
 
 %struct.a = type { i32, %struct.b*, i8, i8, i8, i8, i8*, %struct.b*, i16, i16, i16, i16, i16, i16, i16, i16, i32, i32, i32, i32, i32, i32, i32 }
 %struct.b = type { i8, i8, i8, i8, i32, i16, i16, i32, i32, i32, i32, [16 x i8], [64 x i8], [128 x i8], i32, [68 x i8] }
 
 define void @test(%struct.a* nocapture %dhcp, i16 zeroext %value) #0 {
 ; CHECK-LABEL: test:
 ; CHECK:       @ %bb.0: @ %entry
-; CHECK-NEXT:    .save {r7, lr}
-; CHECK-NEXT:    push {r7, lr}
-; CHECK-NEXT:    ldrh r3, [r0, #20]
-; CHECK-NEXT:    ldr.w lr, [r0, #16]
-; CHECK-NEXT:    lsr.w r12, r1, #8
-; CHECK-NEXT:    adds r2, r3, #1
-; CHECK-NEXT:    strh r2, [r0, #20]
-; CHECK-NEXT:    add.w r2, lr, r3
-; CHECK-NEXT:    strb.w r12, [r2, #240]
 ; CHECK-NEXT:    ldrh r2, [r0, #20]
-; CHECK-NEXT:    ldr.w r12, [r0, #16]
 ; CHECK-NEXT:    adds r3, r2, #1
 ; CHECK-NEXT:    strh r3, [r0, #20]
-; CHECK-NEXT:    add.w r0, r12, r2
+; CHECK-NEXT:    ldr r3, [r0, #16]
+; CHECK-NEXT:    add r2, r3
+; CHECK-NEXT:    lsrs r3, r1, #8
+; CHECK-NEXT:    strb.w r3, [r2, #240]
+; CHECK-NEXT:    ldrh r2, [r0, #20]
+; CHECK-NEXT:    adds r3, r2, #1
+; CHECK-NEXT:    strh r3, [r0, #20]
+; CHECK-NEXT:    ldr r0, [r0, #16]
+; CHECK-NEXT:    add r0, r2
 ; CHECK-NEXT:    strb.w r1, [r0, #240]
-; CHECK-NEXT:    pop {r7, pc}
+; CHECK-NEXT:    bx lr
 entry:
   %shr = lshr i16 %value, 8
   %conv1 = trunc i16 %shr to i8
   %msg_out = getelementptr inbounds %struct.a, %struct.a* %dhcp, i32 0, i32 7
   %0 = load %struct.b*, %struct.b** %msg_out, align 4
   %options_out_len = getelementptr inbounds %struct.a, %struct.a* %dhcp, i32 0, i32 8
   %1 = load i16, i16* %options_out_len, align 4
   %inc = add i16 %1, 1