[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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:
> 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.
> Here OI.dominates(I1, I2) returns true, but DT.dominates(I1, I2) returns false.

Right I now see what's going on: OrderedInstructions does not support passing in uses and DT has special handling for use in PHI nodes. Support for that should be easy to add to OrderedInstructions, which I would strongly encourage to be used here for all instruction level dominance queries. Otherwise each dominates call iterates over the whole basic block in the worst case. Until then, you should be able to use it for `DT.dominates(OpInst, &InsertPoint)`

It would be good to add such a case to CodeMoverUtilsTest.cpp tests. which passes in 2 instructions.


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

Sure. but in the patch you are still using `  const bool MoveForward = OI.dominates(&I, &InsertPoint);` (which is equivalent to the original `DT.dominates`).


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:392
+    if (isSafeToMoveBefore(I, *MovePos, DT, PDT, DI, OI))
       I.moveBefore(MovePos);
   }
----------------
Whitney wrote:
> 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.
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578