[PATCH] [X86] Convert esp-relative movs of function arguments to pushes, step 2

[Michael Kuperstein]

Thanks, Elena!
Will upload a new version.


================
Comment at: lib/Target/X86/X86.h:70
@@ -69,1 +69,3 @@
 
+/// createX86ConvertMovsToPushes - Return a pass that converts movs
+/// that stores function parameters onto the stack into pushes.
----------------
delena wrote:
> I suggest to choose another name, something like optimizeCallFrameForSize
I wasn't happy with the name either, but didn't have any good ideas at the time. 
Will do.

================
Comment at: lib/Target/X86/X86ConvertMovsToPushes.cpp:39
@@ +38,3 @@
+
+cl::opt<bool> NoMovToPush("no-mov-to-push",
+              cl::desc("Avoid function argument mov-to-push transformation"),
----------------
delena wrote:
> I don't think that we really need this knob.
I'd rather keep this knob, it's fairly useful for debugging.
Of course, it's internal only, not exposed to clang.

================
Comment at: lib/Target/X86/X86ConvertMovsToPushes.cpp:113
@@ +112,3 @@
+      if (I->getOpcode() == FrameSetupOpcode)
+        Changed |= adjustCallSequence(MF, *BB, I);
+
----------------
delena wrote:
> If you change instructions inside bb, your iterator may be broken.
As far as I know, MBB iterators aren't invalidated by removing other instructions, and we don't remove the FrameSetup itself.
But it's probably better to keep going from the FrameDestroy instead of the next instruction. 
Will change that.

================
Comment at: lib/Target/X86/X86ConvertMovsToPushes.cpp:212
@@ +211,3 @@
+      unsigned PushOpcode = X86::PUSHi32;
+      if (PushOp.isImm()) {
+        int64_t Val = PushOp.getImm();
----------------
delena wrote:
> It should be immediate, right? Can we have a relocation here?
It can be a relocation, but in that case, isImm() will fail.
Will document that more clearly

================
Comment at: lib/Target/X86/X86ConvertMovsToPushes.cpp:224
@@ +223,3 @@
+      const X86Subtarget &ST = MF.getTarget().getSubtarget<X86Subtarget>();
+      bool SlowPUSHrmm = ST.isAtom() || ST.isSLM();
+      MachineInstr *DefMov = nullptr;
----------------
delena wrote:
> SlowPush should be a property of the target, like slowLea
I agree. Unfortunately, I've run out of bits.
The Subtarget features are 64-bit bitfield, and they're all taken.

================
Comment at: lib/Target/X86/X86RegisterInfo.cpp:508
@@ +507,3 @@
+  if (BasePtr == StackPtr)
+    FIOffset += SPAdj;
+
----------------
And, apparently, this is still wrong, because eliminateCallFramePseudoInstr() may actually adjust the SP by a different amount than what PEI passes as the SPAdj, e.g. due to stack alignment concerns.

http://reviews.llvm.org/D6789

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/