[PATCH] D11749: [X86] When optimizing for size, use POP for small post-call stack clean-up

[Michael Kuperstein]

mkuper added a comment.

Thanks, David!

Do you see any issues with the way I'm choosing the registers? 
I need a sanity check on the "if it's not clobbered and not def'ed, it's free" thing.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1857-1858
@@ +1856,4 @@
+
+  bool Is64Bit = STI.is64Bit();
+  int PopSize = Is64Bit ? 8 : 4;
+  if (Offset % PopSize)
----------------
majnemer wrote:
> Is `PopSize` interestingly different from `SlotSize`?
No, should be the same. :)

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1875
@@ +1874,3 @@
+
+  SmallVector<unsigned, 2> Regs;
+  auto RegMask = Prev->getOperand(1);
----------------
majnemer wrote:
> SmallVector seems like a little overkill, would an array of type `unsigned [2]` make more sense?
Force of habit more than anything else, will change to an array if you prefer that.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1880-1881
@@ +1879,4 @@
+  const TargetRegisterClass *RC = &X86::GR32_NOREX_NOSPRegClass;
+  for (TargetRegisterClass::iterator I = RC->begin(), E = RC->end();
+        I != E; ++I) {
+
----------------
majnemer wrote:
> Can this be a range-based for loop?
Yes.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1908-1910
@@ +1907,5 @@
+
+  // If we found only one free register, but need two, reuse the same one twice.
+  while (Regs.size() < (unsigned)NumPops)
+    Regs.push_back(Regs.back());
+
----------------
majnemer wrote:
> Why bother looking for two free registers if we can count on being able to reuse the first one?  Does it cause a false dependency or something?  More curiosity than anything else.
I'm afraid of a false dependency, yes. I haven't actually measured it. 


http://reviews.llvm.org/D11749