[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Whitney Tsang via Phabricator via llvm-commits]

Whitney marked 3 inline comments as done.
Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:316
 
-  // As I and InsertPoint are control flow equivalent, if I dominates
-  // InsertPoint, then I comes before InsertPoint.
-  const bool MoveForward = DT.dominates(&I, &InsertPoint);
+  const bool MoveForward = OrderedInstructions(&DT).dominates(&I, &InsertPoint);
   if (MoveForward) {
----------------
fhahn wrote:
> Whitney wrote:
> > fhahn wrote:
> > > OrderedInstruction numbers basic block on demand and caches them. In order for that to be effective, it needs to be passed in by the caller. E.g. moveInstsBottomUp would have to create it and pass it in. It should also be used for the `dominates` checks further down the function.
> > > 
> > > But looking at the latest version of the patch, it seems to be completely orthogonal to the main changes. If that's the case, probably best to be split off.
> > The two `dominates` checks further down the function actually want to use `DT.dominates`, e.g. it is not safe to move instruction forward to a InsertPoint where `OI.dominates(InsertPoint, U)` but not `DT.dominates(InsertPoint, U)`. 
> > 
> > The change is needed, since isSafeToMoveBefore uses isControlFlowEquivalent as one of the checks, and the patch changes the behaviour of isControlFlowEquivalent. 
> > The two dominates checks further down the function actually want to use DT.dominates, e.g. it is not safe to move instruction forward to a InsertPoint where OI.dominates(InsertPoint, U) but not DT.dominates(InsertPoint, U).
>  
> I think I am missing something. Shouldn't `OI.dominates(InsertPoint, U)` and ` DT.dominates(InsertPoint, U)` be equivalent?
> 
> > The change is needed, since isSafeToMoveBefore uses isControlFlowEquivalent as one of the checks, and the patch changes the behaviour of isControlFlowEquivalent.
> Again I think I am missing something. Isn't the change in the function just switching DT.dominates to OI.dominates? similar, the change in moveInstructionsToTheBeginning just adds OI?
```
if ()
  I1
else 
  I2
```
Here OI.dominates(I1, I2) returns true, but DT.dominates(I1, I2) returns false.

Notice that isControlFlowEquivalent() is used in isSafeToMoveBefore(). And this patch changes the behaviour of isControlFlowEquivalent(). `DT.dominates` cannot be used to determine if moving forward anymore. 


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:392
+    if (isSafeToMoveBefore(I, *MovePos, DT, PDT, DI, OI))
       I.moveBefore(MovePos);
   }
----------------
fhahn wrote:
> Any modifications to a BB potentially messes up OI. OrderedBasicBlock (used by OrderedInstructions) provides a mechanism to remove instructions, but I think currently there's no way to update the cache to insert instructions at the beginning, so we'd have to remove the cached BB. It seems like this is more work than I initially thought, so maybe it's better to do that as a follow up. Or just keep create the OrderedInstruction object in isSafeToMoveBefore, *if* it can be used by all instruction level dominates queries.
If you agree, I will change back to construct OrderedInstruction in isSafeToMoveBefore for this review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578