[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Whitney Tsang via Phabricator via llvm-commits]

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:
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578