[PATCH] D52827: [LICM] Make LICM able to hoist phis

[John Brawn via Phabricator via llvm-commits]

john.brawn marked 6 inline comments as done.
john.brawn added a comment.

In https://reviews.llvm.org/D52827#1295052, @mkazantsev wrote:

> The patch fails the following test:
>
> ...
>
> Likely some corner case related to conditional branch to the same block for `true` and `false` is not handled. Please make sure that it passes before we can proceed with the patch.


Yes, it's specifically that in such cases the phi in the destination block has the same incoming block appear more than once which I didn't expect. I think the easiest thing to do is to have canHoistPHI return false for such phis.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:636
+    // Now finally hoist BI.
+    ReplaceInstWithInst(
+        HoistTarget->getTerminator(),
----------------
mkazantsev wrote:
> It can be useful to have staitstics on hoisted branches. I'm OK if it goes as a follow-up patch.
I'll add this and also one for the number of created blocks, which seems like it may also be useful.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1369
 static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
-                  ICFLoopSafetyInfo *SafetyInfo, OptimizationRemarkEmitter *ORE) {
-  auto *Preheader = CurLoop->getLoopPreheader();
+                  BasicBlock *Preheader, ICFLoopSafetyInfo *SafetyInfo,
+                  OptimizationRemarkEmitter *ORE) {
----------------
mkazantsev wrote:
> After you've changed the semantics, `Preheader` is no longer the preheader of `CurLoop` (or is it)? If not, please rename it. If yes, why do you need it as parameter? You can just take loop's preheader as it was before.
> 
> In any case, it should be `const BasicBlock *`.
I left it as `Preheader` just because it was simpler not to change it, but yes it makes sense to rename it so I'll do so. It can't be const though, as we're hoisting I into it (i.e. modifying it).


Repository:
  rL LLVM

https://reviews.llvm.org/D52827