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

[Manman Ren]

Stack Alignment: throw error if we can't satisfy the minimal alignment
requirement when creating stack objects in MachineFrameInfo.

Add CreateStackObjectWithMinAlign to throw error when the minimal alignment
can't be achieved and to clamp the alignment when the preferred alignment
can't be achieved. Same is true for CreateVariableSizedObject.
Will not emit error in CreateSpillStackObject or CreateStackObject.

As long as callers of CreateStackObject do not assume the object will be
aligned at the requested alignment, we should not have miscompile since
later optimizations looking at the object's alignment will have the correct
information.




Thanks,
Manman

On Dec 18, 2012, at 12:47 PM, Bob Wilson wrote:

> 
> On Dec 18, 2012, at 11:27 AM, Manman Ren <mren at apple.com> wrote:
> 
>> 
>> There are some discussions in thread
>> Re: [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
> 
> As you can tell, I'm way behind on patch reviews…  Sorry that I missed the other discussion.
> 
> To Chris's comment about LLVMContext::emitError: I'm embarrassed to admit that I'd forgotten that was available for general error messages.  I knew it was there for inline assembly, but didn't make the connection when I saw InstCombine writing a warning directly to errs().  I'll fix that and check for other places that may also need to be updated.
> 
>> 
>> 
>>>> 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.
>> 
>> //////////////////////////////////////////////
>> --> I will pass an extra argument for the minimal alignment and will use emitError if the minimal alignment can't be satisfied and
>> will ignore the case where getPrefTypeAlignment is too large.
>> --> If that does not sound right, please let me know,
>> 
>> Thanks,
>> Manman
>> 
>> On Dec 18, 2012, at 11:15 AM, Chris Lattner <clattner at apple.com> wrote:
>> 
>>> 
>>> On Dec 18, 2012, at 11:06 AM, Bob Wilson <bob.wilson at apple.com> wrote:
>>> 
>>>>>> 
>>>>>> 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.
>>>> 
>>>> In that case, using DEBUG is absolutely the wrong thing.  The warning won't be produced in a compiler built without debugging.  Is there any way you could detect this in the front-end and produce a proper diagnostic?  (I haven't thought about that and haven't looked at the code, so forgive me if that's a stupid idea.)  Otherwise, you just have to print out the warning to "errs()".  In the long term, we need to figure out how to let the optimizer and code-gen produce real diagnostics (tracked at Apple in <rdar://problem/12867368>).
>>> 
>>> Backend errors like this should be reported with LLVMContext::emitError.
>> 
>>> 
>>> -Chris
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121218/d6a58519/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: emit_error.patch
Type: application/octet-stream
Size: 9710 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121218/d6a58519/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121218/d6a58519/attachment-0001.html>