[X86] Allow load folding into PUSH instructions

[mkuper]

Summary

Adds PUSHes to the folding tables.
This also required a fix to the TD definition, since the memory forms of the PUSH/POP instructions did not have the right mayLoad/mayStore flags.

[RKSimon]

Please can you test x86_64 as well - 32-bit pushes on 64-bit targets are rather awkward.....

You've updated the pop definitions as well - is your plan to fix these in a separate patch? In which case maybe not alter them here?

[mkuper]

Unfortunately, aside from 32-bit pushes on 32-bit platforms, I'm not aware of anything that actually generates memory-form pushes at the moment. But if at any point we start doing that, I think having folding from the start would be a good thing. If you can suggest scenarios where that's generated, I'll happily add a test.

Regarding pops - I wasn't originally planning on touching them, but it seems like the current definition is obviously wrong (a POP64rmm may store...), and I'd rather not leave bugs to lurk. :-)
I can fix it in a separate patch, but, as above - I'm not sure how to test that, since we don't actually generate them.

[spatel]

test/CodeGen/X86/fold-push.ll

2 ↗

(On Diff #30117)

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?

[mkuper]

test/CodeGen/X86/fold-push.ll

2 ↗

(On Diff #30117)

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.

[spatel]

lib/Target/X86/X86InstrInfo.cpp

4886 ↗

(On Diff #30117)

Missing period at end of sentence.

4888–4891 ↗

(On Diff #30117)

Spacing looks off here. Hoist Opcode into a local if 80-col is a problem?

lib/Target/X86/X86InstrInfo.td

1078–1080 ↗

(On Diff #30117)

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).

test/CodeGen/X86/fold-push.ll

2 ↗

(On Diff #30117)

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?

[mkuper]

Thanks, Simon, Sanjay.

I've removed the POP part, will commit that separately.
Regarding renaming the feature - will we still need an alias for the old name?

[RKSimon]

Thanks Michael, this looks ready to go. Can you confirm what testing you've done with larger codebases that have a better chance of causing PUSH to be generated?

[spatel]

In D11340#209637, @mkuper wrote:

Regarding renaming the feature - will we still need an alias for the old name?

My understanding is that we make no compatibility guarantees for developer tool command-line options, so unless this is exposed in clang, we're free to change it.

That said, I don't see that policy officially documented, so we should probably confirm with someone else. :)

[mkuper]

Thanks!

Simon, I've done testing with spec cpu2k, did not have any issues.
Sanjay, I've confirmed it with Chandler. Will do the name-change in a separate patch.

llvm/trunk/lib/Target/X86/X86.td

def FeatureSlowDivide32 : SubtargetFeature<"idivl-to-divb",
                                      "HasSlowDivide32", "true",
                                      "Use 8-bit divide for positive values less than 256">;
 def FeatureSlowDivide64 : SubtargetFeature<"idivq-to-divw",
                                      "HasSlowDivide64", "true",
                                      "Use 16-bit divide for positive values less than 65536">;
 def FeaturePadShortFunctions : SubtargetFeature<"pad-short-functions",
                                      "PadShortFunctions", "true",
                                      "Pad short functions">;
+// TODO: This feature ought to be renamed.
+// What it really refers to are CPUs where instruction that cause MSROM
+// lookups are expensive, so alternative sequences should be preferred.
+// The best examples of this are the memory forms of CALL and PUSH
+// instructions, which should be avoided in favor of a MOV + register CALL/PUSH.
 def FeatureCallRegIndirect : SubtargetFeature<"call-reg-indirect",
                                      "CallRegIndirect", "true",
                                      "Call register indirect">;
 def FeatureLEAUsesAG : SubtargetFeature<"lea-uses-ag", "LEAUsesAG", "true",
                                    "LEA instruction needs inputs at AG stage">;
 def FeatureSlowLEA : SubtargetFeature<"slow-lea", "SlowLEA", "true",
                                    "LEA instruction with certain arguments is slow">;
 def FeatureSlowIncDec : SubtargetFeature<"slow-incdec", "SlowIncDec", "true",

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp

static const X86MemoryFoldTableEntry MemoryFoldTable0[] = {
     { X86::MOVUPDrr,    X86::MOVUPDmr,      TB_FOLDED_STORE },
     { X86::MOVUPSrr,    X86::MOVUPSmr,      TB_FOLDED_STORE },
     { X86::MUL16r,      X86::MUL16m,        TB_FOLDED_LOAD },
     { X86::MUL32r,      X86::MUL32m,        TB_FOLDED_LOAD },
     { X86::MUL64r,      X86::MUL64m,        TB_FOLDED_LOAD },
     { X86::MUL8r,       X86::MUL8m,         TB_FOLDED_LOAD },
     { X86::PEXTRDrr,    X86::PEXTRDmr,      TB_FOLDED_STORE },
     { X86::PEXTRQrr,    X86::PEXTRQmr,      TB_FOLDED_STORE },
+    { X86::PUSH16r,     X86::PUSH16rmm,     TB_FOLDED_LOAD },
+    { X86::PUSH32r,     X86::PUSH32rmm,     TB_FOLDED_LOAD },
+    { X86::PUSH64r,     X86::PUSH64rmm,     TB_FOLDED_LOAD },
     { X86::SETAEr,      X86::SETAEm,        TB_FOLDED_STORE },
     { X86::SETAr,       X86::SETAm,         TB_FOLDED_STORE },
     { X86::SETBEr,      X86::SETBEm,        TB_FOLDED_STORE },
     { X86::SETBr,       X86::SETBm,         TB_FOLDED_STORE },
     { X86::SETEr,       X86::SETEm,         TB_FOLDED_STORE },
     { X86::SETGEr,      X86::SETGEm,        TB_FOLDED_STORE },
     { X86::SETGr,       X86::SETGm,         TB_FOLDED_STORE },
     { X86::SETLEr,      X86::SETLEm,        TB_FOLDED_STORE },
MachineInstr *X86InstrInfo::foldMemoryOperandImpl(
     MachineFunction &MF, MachineInstr *MI, unsigned OpNum,
     ArrayRef<MachineOperand> MOs, MachineBasicBlock::iterator InsertPt,
     unsigned Size, unsigned Align, bool AllowCommute) const {
   const DenseMap<unsigned,
                  std::pair<unsigned,unsigned> > *OpcodeTablePtr = nullptr;
   bool isCallRegIndirect = Subtarget.callRegIndirect();
   bool isTwoAddrFold = false;
 
-  // For CPUs that favor the register form of a call,
-  // do not fold loads into calls.
-  if (isCallRegIndirect &&
-    (MI->getOpcode() == X86::CALL32r || MI->getOpcode() == X86::CALL64r))
+  // For CPUs that favor the register form of a call or push,
+  // do not fold loads into calls or pushes, unless optimizing for size
+  // aggressively.
+  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;
 
   unsigned NumOps = MI->getDesc().getNumOperands();
   bool isTwoAddr = NumOps > 1 &&
     MI->getDesc().getOperandConstraint(1, MCOI::TIED_TO) != -1;
 
   // FIXME: AsmPrinter doesn't know how to handle
   // X86II::MO_GOT_ABSOLUTE_ADDRESS after folding.

llvm/trunk/lib/Target/X86/X86InstrInfo.td

 
 let mayStore = 1, SchedRW = [WriteStore] in {
 def PUSH16r  : I<0x50, AddRegFrm, (outs), (ins GR16:$reg), "push{w}\t$reg",[],
                  IIC_PUSH_REG>, OpSize16;
 def PUSH32r  : I<0x50, AddRegFrm, (outs), (ins GR32:$reg), "push{l}\t$reg",[],
                  IIC_PUSH_REG>, OpSize32, Requires<[Not64BitMode]>;
 def PUSH16rmr: I<0xFF, MRM6r, (outs), (ins GR16:$reg), "push{w}\t$reg",[],
                  IIC_PUSH_REG>, OpSize16;
-def PUSH16rmm: I<0xFF, MRM6m, (outs), (ins i16mem:$src), "push{w}\t$src",[],
-                 IIC_PUSH_MEM>, OpSize16;
 def PUSH32rmr: I<0xFF, MRM6r, (outs), (ins GR32:$reg), "push{l}\t$reg",[],
                  IIC_PUSH_REG>, OpSize32, Requires<[Not64BitMode]>;
-def PUSH32rmm: I<0xFF, MRM6m, (outs), (ins i32mem:$src), "push{l}\t$src",[],
-                 IIC_PUSH_MEM>, OpSize32, Requires<[Not64BitMode]>;
 
 def PUSH16i8 : Ii8<0x6a, RawFrm, (outs), (ins i16i8imm:$imm),
                    "push{w}\t$imm", [], IIC_PUSH_IMM>, OpSize16;
 def PUSHi16  : Ii16<0x68, RawFrm, (outs), (ins i16imm:$imm),
                    "push{w}\t$imm", [], IIC_PUSH_IMM>, OpSize16;
 
 def PUSH32i8 : Ii8<0x6a, RawFrm, (outs), (ins i32i8imm:$imm),
                    "push{l}\t$imm", [], IIC_PUSH_IMM>, OpSize32,
                    Requires<[Not64BitMode]>;
 def PUSHi32  : Ii32<0x68, RawFrm, (outs), (ins i32imm:$imm),
                    "push{l}\t$imm", [], IIC_PUSH_IMM>, OpSize32,
                    Requires<[Not64BitMode]>;
 } // mayStore, SchedRW
+
+let mayLoad = 1, mayStore = 1, SchedRW = [WriteRMW] in {
+def PUSH16rmm: I<0xFF, MRM6m, (outs), (ins i16mem:$src), "push{w}\t$src",[],
+                 IIC_PUSH_MEM>, OpSize16;
+def PUSH32rmm: I<0xFF, MRM6m, (outs), (ins i32mem:$src), "push{l}\t$src",[],
+                 IIC_PUSH_MEM>, OpSize32, Requires<[Not64BitMode]>;
+} // mayLoad, mayStore, SchedRW
+
 }
 
 let Defs = [ESP, EFLAGS], Uses = [ESP], mayLoad = 1, hasSideEffects=0,
     SchedRW = [WriteLoad] in {
 def POPF16   : I<0x9D, RawFrm, (outs), (ins), "popf{w}", [], IIC_POP_F>,
                 OpSize16;
 def POPF32   : I<0x9D, RawFrm, (outs), (ins), "popf{l|d}", [], IIC_POP_FD>,
                 OpSize32, Requires<[Not64BitMode]>;

 def POP64rmm: I<0x8F, MRM0m, (outs), (ins i64mem:$dst), "pop{q}\t$dst", [],
                 IIC_POP_MEM>, OpSize32, Requires<[In64BitMode]>;
 } // mayLoad, SchedRW
 let mayStore = 1, SchedRW = [WriteStore] in {
 def PUSH64r  : I<0x50, AddRegFrm, (outs), (ins GR64:$reg), "push{q}\t$reg", [],
                  IIC_PUSH_REG>, OpSize32, Requires<[In64BitMode]>;
 def PUSH64rmr: I<0xFF, MRM6r, (outs), (ins GR64:$reg), "push{q}\t$reg", [],
                  IIC_PUSH_REG>, OpSize32, Requires<[In64BitMode]>;
+} // mayStore, SchedRW
+let mayLoad = 1, mayStore = 1, SchedRW = [WriteRMW] in {
 def PUSH64rmm: I<0xFF, MRM6m, (outs), (ins i64mem:$src), "push{q}\t$src", [],
                  IIC_PUSH_MEM>, OpSize32, Requires<[In64BitMode]>;
-} // mayStore, SchedRW
+} // mayLoad, mayStore, SchedRW
 }
 
 let Defs = [RSP], Uses = [RSP], hasSideEffects = 0, mayStore = 1,
     SchedRW = [WriteStore] in {
 def PUSH64i8   : Ii8<0x6a, RawFrm, (outs), (ins i64i8imm:$imm),
                     "push{q}\t$imm", [], IIC_PUSH_IMM>, Requires<[In64BitMode]>;
 def PUSH64i32  : Ii32S<0x68, RawFrm, (outs), (ins i64i32imm:$imm),
                     "push{q}\t$imm", [], IIC_PUSH_IMM>, OpSize32,

llvm/trunk/test/CodeGen/X86/fold-push.ll

+; RUN: llc < %s -mtriple=i686-windows | FileCheck %s -check-prefix=CHECK -check-prefix=NORMAL
+; RUN: llc < %s -mtriple=i686-windows -mattr=call-reg-indirect | FileCheck %s -check-prefix=CHECK -check-prefix=SLM
+
+declare void @foo(i32 %r)
+
+define void @test(i32 %a, i32 %b) optsize {
+; CHECK-LABEL: test:
+; CHECK: movl [[EAX:%e..]], (%esp)
+; CHECK-NEXT: pushl [[EAX]]
+; CHECK-NEXT: calll
+; CHECK-NEXT: addl $4, %esp
+; CHECK: nop
+; NORMAL: pushl (%esp)
+; SLM: movl (%esp), [[RELOAD:%e..]]
+; SLM-NEXT: pushl [[RELOAD]]
+; CHECK: calll
+; CHECK-NEXT: addl $4, %esp
+  %c = add i32 %a, %b
+  call void @foo(i32 %c)
+  call void asm sideeffect "nop", "~{ax},~{bx},~{cx},~{dx},~{bp},~{si},~{di}"()
+  call void @foo(i32 %c)
+  ret void
+}
+
+define void @test_min(i32 %a, i32 %b) minsize {
+; CHECK-LABEL: test_min:
+; CHECK: movl [[EAX:%e..]], (%esp)
+; CHECK-NEXT: pushl [[EAX]]
+; CHECK-NEXT: calll
+; CHECK-NEXT: addl $4, %esp
+; CHECK: nop
+; CHECK: pushl (%esp)
+; CHECK: calll
+; CHECK-NEXT: addl $4, %esp
+  %c = add i32 %a, %b
+  call void @foo(i32 %c)
+  call void asm sideeffect "nop", "~{ax},~{bx},~{cx},~{dx},~{bp},~{si},~{di}"()
+  call void @foo(i32 %c)
+  ret void
+}