[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
mren at apple.com
Mon Dec 10 14:28:14 PST 2012
On Dec 10, 2012, at 2:16 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> 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.
This means going through the call sites of these functions CreateStackObject … and checking the alignment there instead of
wrapping it up in these functions.
Another option is to pass in an extra argument for the minimal alignment that must be satisfied.
Thanks,
Manman
>
> -Eli
More information about the llvm-commits
mailing list