[PATCH] D11340: [X86] Allow load folding into PUSH instructions

[Sanjay Patel]

spatel added inline comments.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4886
@@ +4885,3 @@
+  // do not fold loads into calls or pushes, unless optimizing for size
+  // aggressively
+  if (isCallRegIndirect && 
----------------
Missing period at end of sentence.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4888-4891
@@ -4885,1 +4887,6 @@
+  if (isCallRegIndirect && 
+      !MF.getFunction()->hasFnAttribute(Attribute::MinSize) &&
+    (MI->getOpcode() == X86::CALL32r || MI->getOpcode() == X86::CALL64r ||
+     MI->getOpcode() == X86::PUSH16r || MI->getOpcode() == X86::PUSH32r ||
+     MI->getOpcode() == X86::PUSH64r))
     return nullptr;
----------------
Spacing looks off here. Hoist Opcode into a local if 80-col is a problem?

================
Comment at: lib/Target/X86/X86InstrInfo.td:1078-1080
@@ -1075,1 +1077,5 @@
+} // mayStore, SchedRW
+let mayLoad = 1, mayStore = 1, SchedRW = [WriteRMW] in {
+def POP64rmm: I<0x8F, MRM0m, (outs), (ins i64mem:$dst), "pop{q}\t$dst", [],
+                IIC_POP_MEM>, OpSize32, Requires<[In64BitMode]>;
 def PUSH64rmm: I<0xFF, MRM6m, (outs), (ins i64mem:$src), "push{q}\t$src", [],
----------------
As suggested earlier, please put the pop changes into their own commit with a checkin comment that explains the change and why there is no test (unless Simon or someone else knows how to test this).

================
Comment at: test/CodeGen/X86/fold-push.ll:2
@@ +1,3 @@
+; RUN: llc < %s -mtriple=i686-windows | FileCheck %s -check-prefix=CHECK -check-prefix=NORMAL
+; RUN: llc < %s -mtriple=i686-windows -mcpu=slm | FileCheck %s -check-prefix=CHECK -check-prefix=SLM
+
----------------
mkuper wrote:
> spatel wrote:
> > Please check the feature flag directly rather than a CPU: -mattr=call-reg-indirect?
> > 
> > Also, can this be loosened to -mtriple=i686-unknown-unknown? There's nothing Windows-specific here?
> I'll change the feature flag, thanks.
> 
> And I'd prefer to leave windows, because the test is somewhat sensitive to the way the stack frame gets constructed. E.g. i686-pc-linux will not work. Given that, I'd rather not rely on unknown-unknown.
Ok.

Seems like 'call-reg-indirect' could use a more general name since it's being used with more than just calls now. But that can be a separate change. Add a TODO comment in the td file?


http://reviews.llvm.org/D11340