[PATCH] D32614: [GVNHoist] Fix: PR32821, add check for anticipability in case of infinite loops

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added a comment.

In https://reviews.llvm.org/D32614#749208, @chandlerc wrote:

> I don't see what would have addressed my concerns.
>
> There still is only a single test case for an infinite loop. There are *several different* CFGs that fail to terminate. You should be testing them.
>
> Examples off the top of my head:
>
> - A non-loop cycle in the CFG.


The code checks for cycle, not loops so it can detect all kinds of explicit cycles as different from other three you mentioned.

> Note that you need to set up the entire hoisting to be *inside* of the outer loop and then make it invalid due to an infinite non-loop cycle that exists within the loop.

Dominance relation takes care of that (see partitionCandidates)

> - A *potential* cycle due to indirect-br
> - An exceptional cycle due to cyclic exception edges from invokes
> - Either indirect-br or exceptions combined with a non-loop cycle.

All the indirect branch targets in the path are excluded in the safety checks (see safeToHoistLDSt and safeToHoistScalar)
So all kinds of cycles are handled.

> Put differently, it would be good to add fairly extensive testing that actively tries to construct corner cases to break the safety checks of this pass as it has had a long history of subtle and difficult to find correctness bugs.

I have bootstrapped clang, ran SPEC2006. The pass was disabled on multiple occasions, I agree, but on several of these occasions the pass was disabled because it exposed bugs in other places, for example:
https://bugs.llvm.org//show_bug.cgi?id=30806
https://bugs.llvm.org//show_bug.cgi?id=32811
https://bugs.llvm.org/show_bug.cgi?id=32153

>From what I understand from the cases you pointed out, this was the only unhandled case.
Thanks for the review.

> I understand that you may naturally have an algorithm that handles all of these without special casing. That is good! But you should *test* them to ensure that when your code sees unexpected or rarely formed control flows it continues to behave well.



> Put differently, it would be good to add fairly extensive testing that actively tries to construct corner cases to break the safety checks of this pass as it has had a long history of subtle and difficult to find correctness bugs.
> 
> Have you or anyone that cares about GVN hoist considered building a tool to synthetically generate random CFGs with some properties to see how the pass behaves? That might help more pro-actively find issues.




https://reviews.llvm.org/D32614