[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Whitney Tsang via Phabricator via llvm-commits]

Whitney marked 2 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) {
----------------
Whitney wrote:
> 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. 
The example should be
```
if (cond)
  I1
if (cond)
  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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578