[PATCH] D149136: [LICM] Don't duplicate instructions just because they're free

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1379
 
-  return CostI == TargetTransformInfo::TCC_Free;
+  return false;
 }
----------------
mkazantsev wrote:
> This function has a very clear semantics of checking cost, and I don't think that legality concerns should be taken into account here. The legality check seems missing here:
> ```
>       // Check to see if we can sink this instruction to the exit blocks
>       // of the loop.  We can do this if the all users of the instruction are
>       // outside of the loop.  In this case, it doesn't even matter if the
>       // operands of the instruction are loop invariant.
>       //
>       bool FreeInLoop = false;
>       bool LoopNestMode = OutermostLoop != nullptr;
>       if (!I.mayHaveSideEffects() &&
>           isNotUsedOrFreeInLoop(I, LoopNestMode ? OutermostLoop : CurLoop,
>                                 SafetyInfo, TTI, FreeInLoop, LoopNestMode) &&
>           canSinkOrHoistInst(I, AA, DT, CurLoop, MSSAU, true, Flags, ORE)) {
>         if (sink(I, LI, DT, CurLoop, SafetyInfo, MSSAU, ORE)) {
>           if (!FreeInLoop) {
>             ++II;
>             salvageDebugInfo(I);
>             eraseInstruction(I, *SafetyInfo, MSSAU);
>           }
>           Changed = true;
>         }
>       }
> ```
> There should be check on legality of duplication somewhere around here.
This is intended as a change of the cost model (i.e. don't do it for anything but GEP), which also fixes the legality issue by dint of this always being legal for GEPs and it not handling anything else anymore.

We don't have a generic way to check legality of this transform, as far as I know.


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

https://reviews.llvm.org/D149136