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

[Manman Ren]

On Dec 2, 2012, at 10:58 PM, Bill Wendling <wendling at apple.com> wrote:

> 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:
Hi Bill,
Sure, I will submit it as two patches and guard with DEBUG.
> 
> 	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:
This can actually happen if the user specified a large alignment or the data type has a large alignment.

Thanks for reviewing,
Manman
> 
> static unsigned clampStackAlignment(bool ShouldClamp, unsigned Align,
>                                    unsigned StackAlign) {
>  assert((!ShouldClamp || Align <= StackAlign) && "whoa! What's going on here?");
>  return Align;
> }
> 
> Otherwise, LGTM.
> 
> -bw
>