[llvm-commits] [PATCH] Fix PR11004: Crash in llc arm backend

[Cameron Zwarich]

With regards to your comment that it is inconsiderate to break the tree intentionally, you are forgetting that this was an experimental feature that never worked outside of small test cases. Neither Clang nor llvm-gcc will generate byval on ARM, so it gets no real testing. It might be more considerate to users to just add an assert saying that ARM byval doesn't work.

I'll let someone else comment on whether we'd rather have a byval that inserts long chains of copies or just disable it entirely. If it is the former, your patch looks fine with the XFAIL of that test removed.

Cameron

On Sep 25, 2011, at 4:16 PM, David Meyer wrote:

> Cameron,
> 
> This one small change (AlwaysInline: false -> true) can keep tip-of-tree functional while Dan works on nested call legalization. It will be easy to reverse this change when his fix goes in.
> 
> IMHO, it is inconsiderate to break the tree intentionally, especially when a temporary fix is so easy. Even if you XFAIL tests, there will be people who run into the regression in other ways, and may be forced to spend time debugging it.
> 
> In addition to my bug, there are two other bugs filed by others which may be related:
> 
> http://llvm.org/bugs/show_bug.cgi?id=10876
> http://llvm.org/bugs/show_bug.cgi?id=10518
> 
> I've attached a new patch, with a TODO and without a new test.
> 
> - pdox
> 
> 
> 
> 
> On Sun, Sep 25, 2011 at 3:26 PM, Cameron Zwarich <zwarich at apple.com> wrote:
> Dan removed support (temporarily, hopefully) for nested call legalization in r138977 and then XFAIL'd the test test/CodeGen/Generic/2010-11-04-BigByval.ll on ARM in r139058. The latter test hits the same infinite recursion as your test case when compiled for ARM.
> 
> Cameron
> 
> On Sep 25, 2011, at 10:46 AM, David Meyer wrote:
> 
>> +Gohman
>> 
>> Cameron,
>> 
>> A nested callseq is currently an invalid DAG representation, and results in a crash or an infinite loop later in compilation. I don't see how it could ever result in a valid program.
>> 
>> If somebody intends to improve ARM performance by making a nested call to memcpy possible, that is great. But in the meantime, these memcpys really need to be inlined so that the compiler works at all.
>> 
>> - David
>> 
>> 
>> On Sun, Sep 25, 2011 at 3:46 AM, Cameron Zwarich <zwarich at apple.com> wrote:
>> The fact that the memcpy is not always inlined is an explicit design decision of the ARM byval implementation. Think of the case of copying a gigantic struct.
>> 
>> The nested legalization of calls it required is unfortunate and has bugs, but I believe Dan Gohman has been cleaning it up a bit.
>> 
>> Unless I am just really out of the loop (which is possible) this is not the right fix.
>> 
>> Sent from my iPhone
>> 
>> On Sep 25, 2011, at 1:34 AM, David Meyer <pdox at google.com> wrote:
>> 
>> > Fixes http://llvm.org/bugs/show_bug.cgi?id=11004
>> >
>> > New test included. Seeking approval to commit.
>> >
>> > Thanks,
>> > pdox
>> > <arm_byval.patch>
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> 
> <arm_byval_2.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110925/49483da6/attachment.html>