[llvm-commits] [PATCH] Add support for functions with VLAs and dynamic stack realignment on x86

[Jakob Stoklund Olesen]

On May 30, 2012, at 2:53 PM, Chad Rosier <mcrosier at apple.com> wrote:

> This attached patch adds support for functions that include VLAs and require dynamic stack realignment on x86.  

Index: test/CodeGen/X86/alloca-align-rounding.ll
+; RUN: llc < %s -march=x86-64 -mtriple=i686-pc-linux | grep and | count 2

Index: test/CodeGen/X86/alloca-align-rounding-32.ll
+; RUN: llc < %s -march=x86 -mtriple=i686-apple-darwin | grep and | count 2

Please convert these tests to FileCheck.


Index: test/CodeGen/X86/dynamic-allocas-VLAs.ll

These tests are great, but I think you are being too specific in many cases. Make sure you aren't testing that LLVM never changes!

For example:

+; CHECK: vmovaps (%rdi), %ymm0
+; CHECK: vmovaps %ymm0, (%rsp)
+; CHECK: leaq 44(%rsp), %rdi
+; CHECK: vzeroupper

- It's not important that RA chose %ymm0.
- It's not important for this test that vzeroupper was inserted.
- The exact stack offset in %rdi is not important (I think).

You should also have some CHECK-NOTs to verify that the stack doesn't get realigned when it shouldn't etc.


Index: lib/Target/X86/X86FrameLowering.cpp

+    // Mark the BasePtr as live-in in every block except the entry.
+    for (MachineFunction::iterator I = llvm::next(MF.begin()), E = MF.end();
+         I != E; ++I)
+      I->addLiveIn(BasePtr);

This should not be necessary. We don't care about liveness of reserved registers. They are assumed to be live everywhere.


Index: lib/Target/X86/X86RegisterInfo.cpp

+    if (CC == CallingConv::GHC) {
+      report_fatal_error(

Could you check for the actual feature here, instead of testing specifically for GHC? Check out TRI::getCallPreservedMask() and the static MO::clobbersPhysReg().

+    Reserved.set(X86::RBX);
+    Reserved.set(X86::EBX);
+    Reserved.set(X86::BX);
+    Reserved.set(X86::BL);
+    Reserved.set(X86::BH);

What about the configurable getBaseRegister()? Look at MCSubRegIterator.

-  if (needsStackRealignment(MF))
+  if (hasBasePointer(MF))
+    BasePtr = getBaseRegister();
+  else if (needsStackRealignment(MF))
     BasePtr = (FrameIndex < 0 ? FramePtr : StackPtr);
   else if (AfterFPPop)
     BasePtr = StackPtr;

Not your fault, but BasePtr is an unfortunate variable name now.

/jakob