[PATCH] D82708: [MachineLICM] NFC - make safety of moving explicitly for IsLoopInvariantInst

[ChenZheng via Phabricator via llvm-commits]

shchenz created this revision.
shchenz added reviewers: hfinkel, efriedma, arsenm, qcolombet, dmgreen, PowerPC.
Herald added subscribers: llvm-commits, wuzish, asbirlea, hiraditya, wdng.
Herald added a project: LLVM.

Currently, `IsLoopInvariantInst` returns true implicitly indicate that all the instruction's operands(virtual registers) are not in the same loop with the instruction itself. So it is always safe to move the instruction since all operands are defined outside of loop.

We want to extend the definition of loop invariant instruction. Even operands of instruction A defined in the same loop with A, A may still be a loop invariant instruction, for example, the operands are rematerializable instructions.

This patch makes safety of moving explicitly in `IsLoopInvariantInst`, so we can extend the definition of loop invariant instruction without considering the above implicitly indicating.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82708

Files:
  llvm/lib/CodeGen/MachineLICM.cpp


Index: llvm/lib/CodeGen/MachineLICM.cpp
===================================================================
--- llvm/lib/CodeGen/MachineLICM.cpp
+++ llvm/lib/CodeGen/MachineLICM.cpp
@@ -216,7 +216,7 @@
 
     bool IsLICMCandidate(MachineInstr &I);
 
-    bool IsLoopInvariantInst(MachineInstr &I);
+    bool IsLoopInvariantInst(MachineInstr &I, bool &IsSafeToMove);
 
     bool HasLoopPHIUse(const MachineInstr *MI) const;
 
@@ -798,11 +798,13 @@
     return;
 
   SmallVector<MachineInstr *, 8> Candidates;
+  bool IsSafeToMove = false;
   for (MachineBasicBlock::instr_iterator I = Preheader->instr_begin();
        I != Preheader->instr_end(); ++I) {
     // We need to ensure that we can safely move this instruction into the loop.
     // As such, it must not have side-effects, e.g. such as a call has.
-    if (IsLoopInvariantInst(*I) && !HasLoopPHIUse(&*I))
+    if (IsLoopInvariantInst(*I, IsSafeToMove) && IsSafeToMove &&
+        !HasLoopPHIUse(&*I))
       Candidates.push_back(&*I);
   }
 
@@ -1047,10 +1049,16 @@
 }
 
 /// Returns true if the instruction is loop invariant.
-/// I.e., all virtual register operands are defined outside of the loop,
-/// physical registers aren't accessed explicitly, and there are no side
-/// effects that aren't captured by the operands or other flags.
-bool MachineLICMBase::IsLoopInvariantInst(MachineInstr &I) {
+/// I.e., all virtual register operands are either defined outside of the loop
+/// or rematerializable instructions, physical registers aren't accessed
+/// explicitly, and there are no side effects that aren't captured by the
+/// operands or other flags.
+/// If the instruction is loop invariant, we can indicate that the instruction
+/// is safe to be moved by setting IsSafeToMove. For example, if a instruction
+/// has an operand defined in same block, it is not safe to move this invariant
+/// instruction.
+bool MachineLICMBase::IsLoopInvariantInst(MachineInstr &I, bool &IsSafeToMove) {
+  IsSafeToMove = true;
   if (!IsLICMCandidate(I))
     return false;
 
@@ -1088,13 +1096,17 @@
     if (!MO.isUse())
       continue;
 
-    assert(MRI->getVRegDef(Reg) &&
-           "Machine instr not mapped for this vreg?!");
+    MachineInstr* Def = MRI->getVRegDef(Reg);
+    assert(Def && "Machine instr not mapped for this vreg?!");
 
-    // If the loop contains the definition of an operand, then the instruction
-    // isn't loop invariant.
-    if (CurLoop->contains(MRI->getVRegDef(Reg)))
-      return false;
+    if (CurLoop->contains(Def)) {
+      // Can not hoist I to other block since it has operand in current block.
+      IsSafeToMove = false;
+      // It this operand is not a rematerializable instruction, I is a loop
+      // variant variable.
+      if (!TII->isTriviallyReMaterializable(*Def, AA))
+        return false;
+    }
   }
 
   // If we got this far, the instruction is loop invariant!
@@ -1371,7 +1383,9 @@
   MBB->insert(Pos, NewMIs[1]);
   // If unfolding produced a load that wasn't loop-invariant or profitable to
   // hoist, discard the new instructions and bail.
-  if (!IsLoopInvariantInst(*NewMIs[0]) || !IsProfitableToHoist(*NewMIs[0])) {
+  bool IsSafeToMove = false;
+  if (!IsLoopInvariantInst(*NewMIs[0], IsSafeToMove) || !IsSafeToMove ||
+      !IsProfitableToHoist(*NewMIs[0])) {
     NewMIs[0]->eraseFromParent();
     NewMIs[1]->eraseFromParent();
     return nullptr;
@@ -1497,8 +1511,10 @@
     ++NumNotHoistedDueToHotness;
     return false;
   }
+  bool IsSafeToMove = false;
   // First check whether we should hoist this instruction.
-  if (!IsLoopInvariantInst(*MI) || !IsProfitableToHoist(*MI)) {
+  if (!IsLoopInvariantInst(*MI, IsSafeToMove) || !IsSafeToMove ||
+      !IsProfitableToHoist(*MI)) {
     // If not, try unfolding a hoistable load.
     MI = ExtractHoistableLoad(MI);
     if (!MI) return false;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D82708.273908.patch
Type: text/x-patch
Size: 3845 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200628/c410c682/attachment.bin>