x86: check hasOpaqueSPAdjustment in canRealignStack

[jfb]

Summary

@rnk pointed out in [1] that x86's canRealignStack logic should match that in CantUseSP from hasBasePointer.

[1]: http://reviews.llvm.org/D11160?id=29713#inline-89350

[jfb]

I'm not sure how to test this. I tried something similar to test/CodeGen/X86/inline-asm-stack-realign3.ll but without success:

; RUN: llc -march=x86 -no-integrated-as < %s | FileCheck %s

declare void @bar(i32* %junk)

define i32 @foo(i1 %cond) {
entry:
  %r = alloca i32, align 128
  store i32 -1, i32* %r, align 128
  br i1 %cond, label %doit, label %skip

doit:
  call void asm sideeffect "xor %esi, %esi\0A\09mov %esi, $0", "=*m,~{esi},~{flags}"(i32* %r)
  %junk = alloca i32
  call void @bar(i32* %junk)
  br label %skip

skip:
  %0 = load i32, i32* %r, align 128
  ret i32 %0
}

This generates the following code:

_foo:                                   # @foo
# BB#0:                                 # %entry
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%esi
	andl	$-128, %esp
	subl	$128, %esp
	movl	%esp, %esi
	movl	$-1, (%esi)
	testb	$1, 8(%ebp)
	je	LBB0_2
# BB#1:                                 # %doit
	#APP
	xorl	%esi, %esi
	movl	%esi, (%esi)
	#NO_APP
	movl	$4, %eax
	calll	__chkstk
	movl	%esp, %eax
	pushl	%eax
	calll	_bar
	addl	$4, %esp
LBB0_2:                                 # %skip
	movl	(%esi), %eax
	leal	-4(%ebp), %esp
	popl	%esi
	popl	%ebp
	retl

Which seems wrong, but is the same as before this patch, so I'm assuming it doesn't trigger my change. Any ideas on how to trigger the change?

[rnk]

Right, your test case is essentially
https://llvm.org/bugs/show_bug.cgi?id=16830.

To get into a case where canRealignStack matters, I think you need:

• No dynamic alloca (otherwise we short-circuit out due to

hasVarSizedObjects())

• No highly-aligned alloca (otherwise we commit to stack realignment early)
• A highly-aligned SSA value (a vector) that must be spilled during

register allocation (is live across a call)

If that doesn't do it, then I think this code is dynamically dead, at least
for now. =/

My understanding is that the original purpose of canRealignStack was to
figure out, part way through register allocation, if the aligned or
unaligned vector load and store instructions should be used to spill to the
stack. In some circumstances, ESI may have already been reserved by inline
asm, a custom calling convention, or something else, so we needed this
check. I think Anton added it as part of
https://llvm.org/bugs/show_bug.cgi?id=951.

Then, at some point, someone decided that realigning the stack was too
expensive, since even if you don't have dynamic allocas, it burns EBP,
wasting a precious x86 GPR. I can't find the PRs or revisions for this, but
I think Chad Rosier tweaked the x86 backend so that it doesn't try to
realign the stack anymore. Instead it just uses the unaligned vector loads
and stores.

So, it sounds like this change is untestable, but I feel like we should
throw it in there, because this condition should really be identical to
CantUseSP in hasBasePointer.

[jfb]

Ah that makes sense. I'm OK with no test if you are, considering I couldn't generate one!

[jfb]

@rnk: ping :-)

I see @mcrosier added reviewers @grosbach, @stoklund. Any particular worry?

[rnk]

yep, lgtm

[mcrosier]

@jfb - no worries; I added Jim and Jakob because I knew they were somewhat familiar with this code (at some point in time many years ago).

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

void X86RegisterInfo::adjustStackMapLiveOutMask(uint32_t *Mask) const {
   for (auto Reg : {X86::EFLAGS, X86::RIP, X86::EIP, X86::IP})
     Mask[Reg / 32] &= ~(1U << (Reg % 32));
 }
 
 //===----------------------------------------------------------------------===//
 // Stack Frame Processing methods
 //===----------------------------------------------------------------------===//
 
+static bool CantUseSP(const MachineFrameInfo *MFI) {
+  return MFI->hasVarSizedObjects() || MFI->hasOpaqueSPAdjustment();
+}
+
 bool X86RegisterInfo::hasBasePointer(const MachineFunction &MF) const {
    const MachineFrameInfo *MFI = MF.getFrameInfo();
 
    if (!EnableBasePointer)
      return false;
 
    // When we need stack realignment, we can't address the stack from the frame
    // pointer.  When we have dynamic allocas or stack-adjusting inline asm, we
    // can't address variables from the stack pointer.  MS inline asm can
    // reference locals while also adjusting the stack pointer.  When we can't
    // use both the SP and the FP, we need a separate base pointer register.
    bool CantUseFP = needsStackRealignment(MF);
-   bool CantUseSP =
-       MFI->hasVarSizedObjects() || MFI->hasOpaqueSPAdjustment();
-   return CantUseFP && CantUseSP;
+   return CantUseFP && CantUseSP(MFI);
 }
 
 bool X86RegisterInfo::canRealignStack(const MachineFunction &MF) const {
   if (!TargetRegisterInfo::canRealignStack(MF))
     return false;
 
   const MachineFrameInfo *MFI = MF.getFrameInfo();
   const MachineRegisterInfo *MRI = &MF.getRegInfo();
 
   // Stack realignment requires a frame pointer.  If we already started
   // register allocation with frame pointer elimination, it is too late now.
   if (!MRI->canReserveReg(FramePtr))
     return false;
 
   // If a base pointer is necessary.  Check that it isn't too late to reserve
   // it.
-  if (MFI->hasVarSizedObjects())
+  if (CantUseSP(MFI))
     return MRI->canReserveReg(BasePtr);
   return true;
 }
 
 bool X86RegisterInfo::hasReservedSpillSlot(const MachineFunction &MF,
                                            unsigned Reg, int &FrameIdx) const {
   // Since X86 defines assignCalleeSavedSpillSlots which always return true
   // this function neither used nor tested.