X86CallFrameOptimization: Bail on win64cc calls

[zvi]

Summary

This transform is not legal for Win64. We already check if MF is
not win64cc, but we also need to check that no transform is applied
on a win64cc call site.
Unfortunately, there is no straightforward way to check the callee's CC,
so we resort to checking the register mask vector for equality to the
Win64-specific entry.

Fixes pr35814

[zvi]

I would appreciate suggestions for alternative solutions.

[rnk]

Here is the MI in question:

# *** IR Dump Before X86 Optimize Call Frame ***:
# Machine code for function pr35814: IsSSA, TracksLiveness

%bb.0: derived from LLVM BB %entry
        %0:gr32 = MOV32rm %rip, 1, %noreg, @ff, %noreg; mem:LD4[@ff](dereferenceable) GR32:%0
        %1:gr32 = MOV32rm %rip, 1, %noreg, @e, %noreg; mem:LD4[@e](dereferenceable) GR32:%1
        ADJCALLSTACKDOWN64 40, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit-def dead %ssp, implicit %rsp, implicit %ssp
        %2:gr64 = MOV64rm %rip, 1, %noreg, @d + 16, %noreg; mem:LD8[<unknown>] GR64:%2
        %3:gr64 = COPY %rsp; GR64:%3
        MOV64mr %3, 1, %noreg, 48, %noreg, killed %2; mem:ST8[<unknown>] GR64:%3,%2
        %4:gr64 = MOV64rm %rip, 1, %noreg, @d + 8, %noreg; mem:LD8[<unknown>] GR64:%4
        MOV64mr %3, 1, %noreg, 40, %noreg, killed %4; mem:ST8[<unknown>] GR64:%3,%4
        %5:gr64 = MOV64rm %rip, 1, %noreg, @d, %noreg; mem:LD8[<unknown>] GR64:%5
        MOV64mr %3, 1, %noreg, 32, %noreg, killed %5; mem:ST8[<unknown>] GR64:%3,%5
        %6:gr32 = MOV32ri64 @d; GR32:%6
        %7:gr64 = SUBREG_TO_REG 0, killed %6, sub_32bit; GR64:%7 GR32:%6
        %rcx = COPY %7; GR64:%7
        %edx = COPY %0; GR32:%0
        %r8d = COPY %1; GR32:%1
        %r9 = COPY %7; GR64:%7
        CALL64pcrel32 @g, <regmask %bh %bl %bp %bpl %bx %di %dil %ebp %ebx %edi %esi %rbp %rbx %rdi %rsi %si %sil %r12 %r13 %r14 %r15 %xmm6 %xmm7 %xmm8 %xmm9 %xmm10 %xmm11 %xmm12 %xmm13 %xmm14 %xmm15 %r12b %r13b and 10 more...>, implicit %rsp, implicit %ssp, implicit %rcx, implicit %edx, implicit %r8d, implicit %r9, implicit-def %rsp, implicit-def %ssp
        ADJCALLSTACKUP64 40, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit-def dead %ssp, implicit %rsp, implicit %ssp
        RET 0

# End machine code for function pr35814.

Isn't this MI buggy? We're adjusting SP down by 40 bytes and storing to SP+48, which could overwrite data. I think the assert is valid.

[zvi]

In D42171#978790, @rnk wrote:

Isn't this MI buggy? We're adjusting SP down by 40 bytes and storing to SP+48, which could overwrite data. I think the assert is valid.

There are a couple of troubling issues seen here:

1. The caller is corrupting its own return address
2. byval is not honored - shouldn't the structs that are passed 'byval' be passed as copies?

This patch does not address these issues.

[rnk]

I think the problem is that passing aggregates to ms_abi functions in wine is broken. It generates invalid MIR. There's a bug for that somewhere (http://llvm.org/PR31362). I'd just dupe this bug against that and leave it alone. As long as the call optimization code doesn't optimize call sequences that have gaps it will naturally ignore win64cc functions.

lib/Target/X86/X86CallFrameOptimization.cpp

private:
   StringRef getPassName() const override { return "X86 Optimize Call Frame"; }
 
   const X86InstrInfo *TII;
   const X86FrameLowering *TFL;
   const X86Subtarget *STI;
   MachineRegisterInfo *MRI;
   unsigned SlotSize;
   unsigned Log2SlotSize;
+  const uint32_t *Win64RegMask;
 };
 
 } // end anonymous namespace
 char X86CallFrameOptimization::ID = 0;
 INITIALIZE_PASS(X86CallFrameOptimization, DEBUG_TYPE,
                 "X86 Call Frame Optimization", false, false)
 
 // This checks whether the transformation is legal.
bool X86CallFrameOptimization::runOnMachineFunction(MachineFunction &MF) {
   MRI = &MF.getRegInfo();
 
   const X86RegisterInfo &RegInfo =
       *static_cast<const X86RegisterInfo *>(STI->getRegisterInfo());
   SlotSize = RegInfo.getSlotSize();
   assert(isPowerOf2_32(SlotSize) && "Expect power of 2 stack slot size");
   Log2SlotSize = Log2_32(SlotSize);
 
+  // Locate the index of CSR_Win64
+  auto Win64RegMaskName = find(RegInfo.getRegMaskNames(), "CSR_Win64");
+  assert(Win64RegMaskName != RegInfo.getRegMaskNames().end() && "Was CSR_Win64 renamed?");
+  Win64RegMask = RegInfo.getRegMasks()[std::distance(
+      RegInfo.getRegMaskNames().begin(), Win64RegMaskName)];
+
   if (skipFunction(MF.getFunction()) || !isLegal(MF))
     return false;
 
   unsigned FrameSetupOpcode = TII->getCallFrameSetupOpcode();
 
   bool Changed = false;
 
   ContextVector CallSeqVector;
void X86CallFrameOptimization::collectCallInfo(MachineFunction &MF,
   auto StackPtrCopyInst = MBB.end();
   // SelectionDAG (but not FastISel) inserts a copy of ESP into a virtual
   // register.  If it's there, use that virtual register as stack pointer
   // instead. Also, we need to locate this instruction so that we can later
   // safely ignore it while doing the conservative processing of the call chain.
   // The COPY can be located anywhere between the call-frame setup
   // instruction and its first use. We use the call instruction as a boundary
   // because it is usually cheaper to check if an instruction is a call than
-  // checking if an instruction uses a register.
-  for (auto J = I; !J->isCall(); ++J)
-    if (J->isCopy() && J->getOperand(0).isReg() && J->getOperand(1).isReg() &&
-        J->getOperand(1).getReg() == StackPtr) {
+  // checking if an instruction uses a register. This look-ahead is an
+  // opportunity to locate the call instruction and check if it is suitable for
+  // the transformation.
+  auto J = I;
+  for (; !J->isCall(); ++J) {
+    if (J->isCopy() && J->getOperand(0).isReg() &&
+        !RegInfo.isPhysicalRegister(J->getOperand(0).getReg()) &&
+        J->getOperand(1).isReg() && J->getOperand(1).getReg() == StackPtr) {
+      assert(StackPtrCopyInst == MBB.end() &&
+             "More than one stack-pointer copy?!");
       StackPtrCopyInst = J;
       Context.SPCopy = &*J++;
       StackPtr = Context.SPCopy->getOperand(0).getReg();
-      break;
     }
+    assert(J != MBB.end() && "Did not see any call");
+  }
+  auto TheCall = J;
+
+  // This transform is not legal for Win64. We already checked if MF is not
+  // win64cc, but we also need to check that no transform is applied on a
+  // win64cc call site. Unfortunately, there is no straightforward way to check
+  // the callee's CC, so we resort to checking the register mask for the
+  // Win64-specific entry.
+  // FIXME: Perform a more robust check on the call for win64cc.
+  auto O = find_if(TheCall->operands(), [](const MachineOperand &Op) {
+    return Op.getType() == MachineOperand::MO_RegisterMask;
+  });
+  assert(O != TheCall->operands_end() && "Call instruction has no RegisterMask operand");
+  // Bail if call is win64cc.
+  if (O->getRegMask() == Win64RegMask)
+    return;
 
   // Scan the call setup sequence for the pattern we're looking for.
   // We only handle a simple case - a sequence of store instructions that
   // push a sequence of stack-slot-aligned values onto the stack, with
   // no gaps between them.
   if (MaxAdjust > 4)
     Context.ArgStoreVector.resize(MaxAdjust, nullptr);
 
for (const MachineOperand &MO : I->uses()) {
       if (!MO.isReg())
         continue;
       unsigned int Reg = MO.getReg();
       if (RegInfo.isPhysicalRegister(Reg))
         UsedRegs.insert(Reg);
     }
   }
 
-  --I;
-
   // We now expect the end of the sequence. If we stopped early,
   // or reached the end of the block without finding a call, bail.
-  if (I == MBB.end() || !I->isCall())
+  if (std::prev(I) != TheCall)
     return;
 
-  Context.Call = &*I;
-  if ((++I)->getOpcode() != TII->getCallFrameDestroyOpcode())
+  Context.Call = &*TheCall;
+  if (I->getOpcode() != TII->getCallFrameDestroyOpcode())
     return;
 
   // Now, go through the vector, and see that we don't have any gaps,
   // but only a series of storing instructions.
   auto MMI = Context.ArgStoreVector.begin(), MME = Context.ArgStoreVector.end();
   for (; MMI != MME; ++MMI, Context.ExpectedDist += SlotSize)
     if (*MMI == nullptr)
       break;

test/CodeGen/X86/movtopush.ll

 ; NOPUSH-NEXT: andl  $0, 48(%esp)  
 ; NOPUSH-NEXT: calll  _eightparams64
 ; NOPUSH-NEXT: addl  $64, %esp     
 define void @pr34863_64(i64 %x) minsize nounwind {
 entry:
   tail call void @eightparams64(i64 %x, i64 %x, i64 %x, i64 %x, i64 %x, i64 %x, i64 0, i64 -1)
   ret void
 }
+
+
+%struct.anon = type { %struct.anon.0 }
+%struct.anon.0 = type { %struct.anon.1 }
+%struct.anon.1 = type { i16, %struct.anon.2 }
+%struct.anon.2 = type { %struct.anon.3 }
+%struct.anon.3 = type { i8*, i32 }
+
+@d = common global %struct.anon zeroinitializer, align 8
+@ff = common local_unnamed_addr global i32 0, align 4
+@e = common local_unnamed_addr global i32 0, align 4
+
+; Bail out gracefully in presence of win64cc calls
+define void @pr35814() local_unnamed_addr {
+entry:
+  %0 = load i32, i32* @ff, align 4
+  %1 = load i32, i32* @e, align 4
+  tail call win64cc void bitcast (void (...)* @g to void (%struct.anon*, i32, i32, %struct.anon*, %struct.anon*)*)(%struct.anon* byval nonnull align 8 @d, i32 %0, i32 %1, %struct.anon* byval nonnull align 8 @d, %struct.anon* byval nonnull align 8 @d) #2
+  ret void
+}
+
+declare win64cc void @g(...) local_unnamed_addr