[PATCH] D80084: [CodeMoverUtils] Use dominator tree level to decide the direction of code motion

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:323-337
   if (MoveForward) {
     // When I is being moved forward, we need to make sure the InsertPoint
     // dominates every users. Or else, a user may be using an undefined I.
     for (const Use &U : I.uses())
       if (auto *UserInst = dyn_cast<Instruction>(U.getUser()))
         if (UserInst != &InsertPoint && !DT.dominates(&InsertPoint, U))
           return false;
----------------
I'd feel a lot better about this code if this code were just

```
  if (!DT.dominates(&InsertPoint, &I))
    for (const Use &U : I.uses())
      if (auto *UserInst = dyn_cast<Instruction>(U.getUser()))
        if (UserInst != &InsertPoint && !DT.dominates(&InsertPoint, U))
          return false;
  if (!DT.dominates(&I, &InsertPoint))
    for (const Value *Op : I.operands())
      if (auto *OpInst = dyn_cast<Instruction>(Op))
        if (&InsertPoint == OpInst || !DT.dominates(OpInst, &InsertPoint))
          return false;
```

which is more "obviously correct". The outer dominance checks here are just an optimization to handle obvious cases, but

```
    for (const Use &U : I.uses())
      if (auto *UserInst = dyn_cast<Instruction>(U.getUser()))
        if (UserInst != &InsertPoint && !DT.dominates(&InsertPoint, U))
          return false;
    for (const Value *Op : I.operands())
      if (auto *OpInst = dyn_cast<Instruction>(Op))
        if (&InsertPoint == OpInst || !DT.dominates(OpInst, &InsertPoint))
          return false;
```

would work as well.

In this case the `MoveForward` variable would only be needed for the collectInstructionsInBetween() check below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80084