[llvm-commits] [llvm] r169197 - in /llvm/trunk: include/llvm/CodeGen/MachineFrameInfo.h include/llvm/Target/TargetFrameLowering.h lib/CodeGen/MachineFunction.cpp test/CodeGen/ARM/alloc-no-stack-realign.ll

[Eli Friedman]

On Mon, Dec 10, 2012 at 2:03 PM, Manman Ren <mren at apple.com> wrote:
>
> On Dec 10, 2012, at 1:38 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>
>> On Mon, Dec 3, 2012 at 4:52 PM, Manman Ren <mren at apple.com> wrote:
>>> Author: mren
>>> Date: Mon Dec  3 18:52:33 2012
>>> New Revision: 169197
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=169197&view=rev
>>> Log:
>>> 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.
>>>
>>> rdar://12713765
>>>
>>> Added:
>>>    llvm/trunk/test/CodeGen/ARM/alloc-no-stack-realign.ll
>>> Modified:
>>>    llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h
>>>    llvm/trunk/include/llvm/Target/TargetFrameLowering.h
>>>    llvm/trunk/lib/CodeGen/MachineFunction.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h?rev=169197&r1=169196&r2=169197&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h (original)
>>> +++ llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h Mon Dec  3 18:52:33 2012
>>> @@ -221,8 +221,11 @@
>>>   /// just allocate them normally.
>>>   bool UseLocalStackAllocationBlock;
>>>
>>> +  /// Whether the "realign-stack" option is on.
>>> +  bool RealignOption;
>>> public:
>>> -    explicit MachineFrameInfo(const TargetFrameLowering &tfi) : TFI(tfi) {
>>> +    explicit MachineFrameInfo(const TargetFrameLowering &tfi, bool RealignOpt)
>>> +    : TFI(tfi), RealignOption(RealignOpt) {
>>>     StackSize = NumFixedObjects = OffsetAdjustment = MaxAlignment = 0;
>>>     HasVarSizedObjects = false;
>>>     FrameAddressTaken = false;
>>>
>>> Modified: llvm/trunk/include/llvm/Target/TargetFrameLowering.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Target/TargetFrameLowering.h?rev=169197&r1=169196&r2=169197&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/Target/TargetFrameLowering.h (original)
>>> +++ llvm/trunk/include/llvm/Target/TargetFrameLowering.h Mon Dec  3 18:52:33 2012
>>> @@ -47,11 +47,12 @@
>>>   unsigned StackAlignment;
>>>   unsigned TransientStackAlignment;
>>>   int LocalAreaOffset;
>>> +  bool StackRealignable;
>>> public:
>>>   TargetFrameLowering(StackDirection D, unsigned StackAl, int LAO,
>>> -                      unsigned TransAl = 1)
>>> +                      unsigned TransAl = 1, bool StackReal = true)
>>>     : StackDir(D), StackAlignment(StackAl), TransientStackAlignment(TransAl),
>>> -      LocalAreaOffset(LAO) {}
>>> +      LocalAreaOffset(LAO), StackRealignable(StackReal) {}
>>>
>>>   virtual ~TargetFrameLowering();
>>>
>>> @@ -76,6 +77,12 @@
>>>     return TransientStackAlignment;
>>>   }
>>>
>>> +  /// isStackRealignable - This method returns whether the stack can be
>>> +  /// realigned.
>>> +  bool isStackRealignable() const {
>>> +    return StackRealignable;
>>> +  }
>>> +
>>>   /// getOffsetOfLocalArea - This method returns the offset of the local area
>>>   /// from the stack pointer on entrance to a function.
>>>   ///
>>>
>>> Modified: llvm/trunk/lib/CodeGen/MachineFunction.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineFunction.cpp?rev=169197&r1=169196&r2=169197&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/MachineFunction.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/MachineFunction.cpp Mon Dec  3 18:52:33 2012
>>> @@ -58,7 +58,8 @@
>>>   else
>>>     RegInfo = 0;
>>>   MFInfo = 0;
>>> -  FrameInfo = new (Allocator) MachineFrameInfo(*TM.getFrameLowering());
>>> +  FrameInfo = new (Allocator) MachineFrameInfo(*TM.getFrameLowering(),
>>> +                                               TM.Options.RealignStack);
>>>   if (Fn->getFnAttributes().hasAttribute(Attributes::StackAlignment))
>>>     FrameInfo->ensureMaxAlignment(Fn->getAttributes().
>>>                                   getFnAttributes().getStackAlignment());
>>> @@ -448,15 +449,31 @@
>>> /// ensureMaxAlignment - Make sure the function is at least Align bytes
>>> /// aligned.
>>> void MachineFrameInfo::ensureMaxAlignment(unsigned Align) {
>>> +  if (!TFI.isStackRealignable() || !RealignOption)
>>> +    assert(Align <= TFI.getStackAlignment() &&
>>> +           "For targets without stack realignment, Align is out of limit!");
>>>   if (MaxAlignment < Align) MaxAlignment = Align;
>>> }
>>>
>>> +/// clampStackAlignment - Clamp the alignment if requested and emit a warning.
>>> +static inline unsigned clampStackAlignment(bool ShouldClamp, unsigned Align,
>>> +                                           unsigned StackAlign) {
>>> +  if (!ShouldClamp || Align <= StackAlign)
>>> +    return Align;
>>> +  DEBUG(dbgs() << "Warning: requested alignment " << Align
>>> +               << " exceeds the stack alignment " << StackAlign
>>> +               << " when stack realignment is off" << '\n');
>>> +  return StackAlign;
>>> +}
>>> +
>>> /// CreateStackObject - Create a new statically sized stack object, returning
>>> /// a nonnegative identifier to represent it.
>>> ///
>>> int MachineFrameInfo::CreateStackObject(uint64_t Size, unsigned Alignment,
>>>                       bool isSS, bool MayNeedSP, const AllocaInst *Alloca) {
>>>   assert(Size != 0 && "Cannot allocate zero size stack objects!");
>>> +  Alignment = clampStackAlignment(!TFI.isStackRealignable() || !RealignOption,
>>> +                                  Alignment, TFI.getStackAlignment());
>>>   Objects.push_back(StackObject(Size, Alignment, 0, false, isSS, MayNeedSP,
>>>                                 Alloca));
>>>   int Index = (int)Objects.size() - NumFixedObjects - 1;
>>> @@ -471,6 +488,8 @@
>>> ///
>>> int MachineFrameInfo::CreateSpillStackObject(uint64_t Size,
>>>                                              unsigned Alignment) {
>>> +  Alignment = clampStackAlignment(!TFI.isStackRealignable() || !RealignOption,
>>> +                                  Alignment, TFI.getStackAlignment());
>>>   CreateStackObject(Size, Alignment, true, false);
>>>   int Index = (int)Objects.size() - NumFixedObjects - 1;
>>>   ensureMaxAlignment(Alignment);
>>
>> Isn't this just going to cause a different miscompile if the alignment
>> of the stack object actually matters?  It should be a fatal error to
>> try and create a stack slot we can't sufficiently align.
>
> This patch will emit a warning if the alignment is actually clamped.
> It is hard to say whether we should emit a warning or throw a fatal error.

IMO, it should be an error; users will never see the message otherwise.

> When creating stack object, we use Align
> Align = std::max((unsigned)TLI.getDataLayout()->getPrefTypeAlignment(Ty),
>                    AI->getAlignment());
> The preferred alignment may be too big for the target, and it makes sense to give a warning instead of a fatal error.

If we can't realign the stack, we should print an error if
AI->getAlignment() is too large and simply ignore the return of
getPrefTypeAlignment if it's too large.

-Eli