[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

[Manman Ren]

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.

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.

Thanks,
Manman

> 
> -Eli