[PATCH] D70213: [ModuloSchedule] Fix a bug in experimental expander during prologue/epilogue stitching.

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
Comment at: llvm/lib/CodeGen/ModuloSchedule.cpp:1660
   InsertPt = DestBB->getFirstNonPHI();
-  for (MachineInstr &MI : SourceBB->phis()) {
-    MachineInstr *NewMI = MF.CloneMachineInstr(&MI);
-    DestBB->insert(InsertPt, NewMI);
-    Register OrigR = MI.getOperand(0).getReg();
-    Register R = MRI.createVirtualRegister(MRI.getRegClass(OrigR));
-    NewMI->getOperand(0).setReg(R);
-    NewMI->getOperand(1).setReg(OrigR);
-    NewMI->getOperand(2).setMBB(*DestBB->pred_begin());
-    Remaps[OrigR] = R;
-    CanonicalMIs[NewMI] = CanonicalMIs[&MI];
-    BlockMIs[{DestBB, CanonicalMIs[&MI]}] = NewMI;
-  }
   for (auto I = DestBB->getFirstNonPHI(); I != DestBB->end(); ++I)
+    for (MachineOperand &MO : I->uses()) {
----------------
What's the rationale for this change? Is this just to avoid creating a phi that we destroy later?

If so, I'd prefer to keep the logic simple over optimal. We can always clean up dead nodes easily.


================
Comment at: llvm/lib/CodeGen/ModuloSchedule.cpp:1767
+        // chain to find the right value.
+        MachineInstr *CanonicalUse = CanonicalMIs[Use];
+        if (CanonicalUse->isPHI()) {
----------------
The nesting is getting too deep here, let's move this into a helper function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70213