[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:320
+  DT.updateDFSNumbers();
+  const bool MoveForward = OI.dfsBefore(&I, &InsertPoint);
   if (MoveForward) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578