[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:
> 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?
I don't think that bfsBefore() will help either. As a simple case, if you have an if/else, and query moving an instruction between the if and the else block, then one of them is going to have a lower BFS number, but you still can't determine "move forward" or "move backward" based on that. (In this case, the outcome should be either "no move possible" or the move has to consist of both move forward and move backward, or move backward and move forward.)

For the purpose of the dominance checks below, it would be sufficient to just check dominance for both uses and operands, independent of "MoveForward", as the use/op dominance always needs to hold. The main problem is the "collectInstructionsInBetween" below, which needs to know the scan direction.

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

OrderedInstructions is now a thin wrapper around DominatorTree and Instruction::comesBefore(). It used to be caching analysis for local dominance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578