[PATCH] D17000: [AArch64] Reduce number of callee-save save/restores.

[Tim Northover via llvm-commits]

t.p.northover added a comment.

Thanks for rewriting it to accommodate MachO. I've got a couple of minor suggestions, but only saw one likely bug (the unconditional register increment).

Cheers.

Tim.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:702-707
@@ +701,8 @@
+
+    if (AArch64::GPR64RegClass.contains(RPI.Reg1))
+      RPI.IsGPR = true;
+    else if (AArch64::FPR64RegClass.contains(RPI.Reg1))
+      RPI.IsGPR = false;
+    else
+      llvm_unreachable("Unexpected callee saved register!");
+
----------------
Possibly:

    assert(AArch64::GPR64RegClass.contains(RPI.Reg1) ||
           AArch64::FPR64RegClass.contains(RPI.Reg1));
    RPI.IsGPR =  AArch64::GPR64RegClass.contains(RPI.Reg1);

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:760
@@ +759,3 @@
+  RegPairInfo &LastPair = RegPairs.back();
+  assert(AFI->getCalleeSavedStackSize() % 8 == 0);
+  LastPair.Offset = AFI->getCalleeSavedStackSize() / 8;
----------------
Did you decide a stack size == 8 (mod 16) was allowed in some circumstances?

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:957
@@ -903,1 +956,3 @@
+        if (Subtarget.isTargetMachO())
+          UnspilledCSGPRPaired = PairedReg;
       }
----------------
The logic would be marginally simpler if you set this unconditionally and then used the MachO test when actually using it.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1008
@@ +1007,3 @@
+        SavedRegs.set(UnspilledCSGPRPaired);
+        ++NumRegsSpilled;
+      }
----------------
This will be incremented even if UnspilledCSGPRPaired was already spilled.


http://reviews.llvm.org/D17000