PR5373

Hello,

This patch fixes pr5373, testcase of course attached.

-Jakub

5373.patch (1.5 KB)

Committed in revision 89758.

Many thanks,
Edward.

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

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

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

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 :slight_smile:

Dan, can you look at this patch?

Thanks
-Jakub

pr5373-2.patch (1.63 KB)

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