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

[Reid Kleckner]

lgtm

I still think forming pushes prior to isel is the way to go long term. It's a lot easier to convert pushes to 'load, SP adjust, store' than it is to go the other way.


================
Comment at: include/llvm/Target/TargetFrameLowering.h:196
@@ -195,1 +195,3 @@
 
+  // needsFrameIndexResolution - do we need to perform FI resolution for
+  // this function. Normally, this is required only when the function
----------------
"- Do" uppercase

================
Comment at: lib/Target/X86/X86ConvertMovsToPushes.cpp:100
@@ +99,3 @@
+  int FrameDestroyOpcode = TII->getCallFrameDestroyOpcode();
+  for (MachineFunction::iterator BB = MF.begin(), E = MF.end(); BB != E; ++BB) {
+    bool InsideFrameSequence = false;
----------------
Can this be `for (MachineBasicBlock &BB : *MF) {`?

================
Comment at: lib/Target/X86/X86ConvertMovsToPushes.cpp:102
@@ +101,3 @@
+    bool InsideFrameSequence = false;
+    for (MachineBasicBlock::iterator I = BB->begin(); I != BB->end(); ++I) {
+      if (I->getOpcode() == FrameSetupOpcode) {
----------------
Ditto, `for (MachineInstr &MI : BB) {` ?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:1713-1725
@@ +1712,15 @@
+  
+  // FIXME: This is horrible.
+  // The problem is that to know whether a call adjusts the stack, we
+  // need information that is bound to the following ADJCALLSTACKUP
+  // pseudo. This leaves us with a choice:
+  // 1) Look for the next ADJCALLSTACKUP, like the code below. This
+  // works, but certainly doesn't belong here.
+  // 2) Factor out the code that makes the decision on whether a function
+  // has callee-pop, and call it here as well as when creating the 
+  // ADJCALLSTACKUP. This is also insanely ugly.
+  // 3) Don't use SP adjustments (in practice, disable mov -> push 
+  // tranformation) in the presence of callee-pop. This would be 
+  // unfortunate, but may be a good temporary solution.
+  // 4) ?
+  if (MI->isCall()) {
----------------
I would shorten this to just something like "look for the ADJCALLSTACKUP instr that follows the call".

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:1717-1718
@@ +1716,4 @@
+  // pseudo. This leaves us with a choice:
+  // 1) Look for the next ADJCALLSTACKUP, like the code below. This
+  // works, but certainly doesn't belong here.
+  // 2) Factor out the code that makes the decision on whether a function
----------------
mkuper wrote:
> rnk wrote:
> > mkuper wrote:
> > > rnk wrote:
> > > > This is the best thing I can think of at the moment. =/
> > > Too bad. :-\ 
> > > 
> > > So you think I should commit with this code as is?
> > > This shouldn't be a huge problem in terms of compile-time (since I'm looking only until the next call, it can't go quadratic), but it's insanely ugly.
> > Yeah, if we go with this MI pass approach to mov -> push conversion, then we'll have to keep this ADJCALLSTACKUP scan. We aren't going to move the callee cleanup stack adjustment onto the CALL instr without major changes.
> This will have to happen regardless of the MI pass vs. DAG approach.
> 
> I mean, I still think doing it on the DAG is unfeasible, but even if we could do that, it wouldn't help. 
> This code is used for the case where fi resolution needs to handle a a sequence where there is a fi reference between the call and the adjcallstackup, with callee cleanup for the call.
> This is just a side effect of making canSimplifyCallFramePseudos return false.
I was imagining in the DAG LowerCall implementation we emit FrameIndex operands with some kind of SP offset to indicate the current stack level. We'd end up with MI looking like this:
```
ADJCALLSTACKDOWN32 <N> ; N is <size-of-args> % <stack-alignment>, which is usually zero
PUSH32rmm <fi> <sp offset, N>
PUSH32rmm <fi> <sp offset, N + 4>
PUSH32rmm <fi> <sp offset, N + 8>
CALL32rm <fi> <sp offset, N + 12>
ADJCALLSTACKUP32 <N + 12>
```
The main thing is that if we commit to pushes instead of movs at DAG time, it's impossible for the push conversion to fail for hard to diagnose reasons.

It looks like the frame index MachineOperand type has an unused offset field.

http://reviews.llvm.org/D6789

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