[llvm-commits] [PATCH] Stack Alignment: clamp the alignment of stack objects in MachineFrameInfo

[Bill Wendling]

On Nov 30, 2012, at 2:21 PM, Manman Ren <mren at apple.com> wrote:

>> On Nov 30, 2012, at 12:12 PM, Manman Ren <mren at apple.com> wrote:
>> 
>>> 
>>> Hi all,
>>> 
>>> Stack Alignment: when creating stack objects in MachineFrameInfo, make sure
>>> the alignment is clamped to TargetFrameLowering.getStackAlignment if the target
>>> does not support stack realignment or the option "realign-stack" is off.
>>> 
>>> This will cause miscompile if the address is treated as aligned and add is
>>> replaced with or in DAGCombine.
>>> 
>>> Added a bool StackRealignable to TargetFrameLowering to check whether stack
>>> realignment is implemented for the target. Also added a bool RealignOption
>>> to MachineFrameInfo to check whether the option "realign-stack" is on.
>>> 
>>> Moved a few functions from MachineFrameInfo.h to .cpp and added clamping of
>>> alignment to those functions.
>>> 
Hi Manman,

Could you move the functions from MachineFrameInfo.h first and then add the clamping code? Also, the debug output you emit should be within 'DEBUG()' macro. Like this:

	DEBUG(dbgs() << "Warning: requested alignment " << Align
	         << " exceeds the stack alignment " << StackAlign
	         << " when stack realignment is off\n");

Or better yet make it an 'assert' if you don't expect this situation to ever occur:

static unsigned clampStackAlignment(bool ShouldClamp, unsigned Align,
                                    unsigned StackAlign) {
  assert((!ShouldClamp || Align <= StackAlign) && "whoa! What's going on here?");
  return Align;
}

Otherwise, LGTM.

-bw