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

[Chad Rosier]

On May 30, 2012, at 4:33 PM, Jakob Stoklund Olesen wrote:

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

Done.

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

Done, done, done..  Thanks for the suggestions.

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

Removed.

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

I'm not sure I follow.  Could I call TRI::getCalleeSavedRegs() and then check to see if this is a non-empty set?  Then I could select one of these registers or would it be better to define the register statically (currently, X86::RBX)?

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

I'm using the MCSubRegIterator now, but I'm not sure this addresses your comment.  Is that what you were suggesting?

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