[LLVMdev] PR5373

[Dan Gohman]

Hello,

First, the included testcase included with this patch passes
without the rest of the patch applied. Please make the check more
strict so that it actually tests the bug.

Second, I don't believe this fix is correct. The header is marked
visited because a branch back to the loop header is one of the
things this code is searching for, so it shouldn't continue
recursing at that point.

I believe isTrivialLoopExitBlock is returning correct results in
this testcase, at least according to what its comments say that
it should be doing. The exit really is trivial, according to the
criteria in the comments. This suggests that the bug is in the
code that calls isTrivialLoopExitBlock.  So instead of trying to
get isTrivialLoopExitBlock to say "no" for this case, hiding the
bug, it would be better to find the actual bug and fix it instead.

Thanks,

Dan

On Dec 6, 2009, at 9:06 AM, Jakub Staszak wrote:

> Hello,
> 
> Yeah, sorry, you are right. My new idea is that only one ExitBB is found because Header ("for.body") is already marked as visited. I'm pretty sure that someone had a good reason to do this that way, but I can't find it out :)
> 
> Dan, can you look at this patch?
> 
> Thanks
> -Jakub
> 
> <pr5373-2.patch>
> 
> On Nov 30, 2009, at 7:52 PM, Dan Gohman wrote:
> 
>> Hello,
>> 
>> Simply removing that "return true" causes the code to search
>> blocks outside of loops for side effects. That's not
>> what the code is supposed to do.
>> 
>> Dan
>> 
>> On Nov 27, 2009, at 3:01 AM, Jakub Staszak wrote:
>> 
>>> Hi,
>>> 
>>> Because of this "return true" not every block was visited and only one ExitBB was found (instead of two). Thus, loop was optimized as a trivial one, which was wrong.
>>> 
>>> -Jakub
>>> 
>>> On Nov 24, 2009, at 2:28 PM, Dan Gohman wrote:
>>> 
>>>> Hello,
>>>> 
>>>> I haven't studied this in detail, but at a first look this makes the code inconsistent with the associated comments. Why should the code continue recursing past a loop exit?
>>>> 
>>>> Dan
>>>> 
>>>> On Nov 23, 2009, at 4:43 AM, Jakub Staszak <kuba at gcc.gnu.org> wrote:
>>>> 
>>>>> Hello,
>>>>> 
>>>>> This patch fixes pr5373, testcase of course attached.
>>>>> 
>>>>> -Jakub
>>>>> <5373.patch>
>>>>> _______________________________________________
>>>>> LLVM Developers mailing list
>>>>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>> 
>