[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Whitney Tsang via Phabricator via llvm-commits]

Whitney marked an inline comment as done.
Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:320
+  DT.updateDFSNumbers();
+  const bool MoveForward = OI.dfsBefore(&I, &InsertPoint);
   if (MoveForward) {
----------------
nikic wrote:
> Whitney wrote:
> > fhahn wrote:
> > > nit: I think it would be clearer to use OI.dominates, like t line 333. I think with OI.dominates, the DFS numbers should be updated on demand, automatically.
> > I intensionally use OI.dfsBefore instead of OI.dominates, as they are not equivalent.  Unit test IsSafeToMoveTest2 illustrate the need. 
> Just stumbled across this while trying to eliminate the OrderedInstructions use: The use of `dfsBefore` here doesn't really make sense. The dfsBefore check can luck out and give you a correct result if both I and InsertPoint *happen* to be on the same DFS path, but there's no guarantee that this is the case. You can easily see this by swapping the block order in the IsSafeToMoveTest2 test case:
> 
> ```
> diff --git llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
> index cf764bf76f06..dc70c6c52717 100644
> --- llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
> +++ llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
> @@ -555,13 +555,13 @@ TEST(CodeMoverUtils, IsSafeToMoveTest2) {
>    std::unique_ptr<Module> M =
>        parseIR(C, R"(define void @foo(i1 %cond, i32 %op0, i32 %op1) {
>                   entry:
> -                   br i1 %cond, label %if.then.first, label %if.end.first
> +                   br i1 %cond, label %if.end.first, label %if.then.first
>                   if.then.first:
>                     %add = add i32 %op0, %op1
>                     %user = add i32 %add, 1
>                     br label %if.end.first
>                   if.end.first:
> -                   br i1 %cond, label %if.then.second, label %if.end.second
> +                   br i1 %cond, label %if.end.second, label %if.then.second
>                   if.then.second:
>                     %sub_op0 = add i32 %op0, 1
>                     %sub = sub i32 %sub_op0, %op1
> ```
> 
> This will make both queries return true instead of false, which is obviously not right.
> 
> I don't know how to make this code correct though, or how a notion of "forward" or "backward" on a graph would be rigorously defined.
> 
> You can of course make the code conservatively correct by requiring that I dominates InsertPoint or InsertPoint dominates I, but from what I understood you're specifically interested in cases where this is not the case.
You are right, it is incorrect to use `dfsBefore` here. I am thinking to create a `bfsBefore`, which should gives correct result no matter for the original `IsSafeToMoveTest2` or the modified `IsSafeToMoveTest2`. 

Why do you want to eliminate the use of `OrderedInstructions`, should I add `bfsBefore` there?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71578/new/

https://reviews.llvm.org/D71578