[Peephole] Check instructions from CopyMIs are still COPY

[weiguozhi]

Function foldRedundantCopy records COPY instructions in CopyMIs and uses it later. But other optimizations may delete or modify it. So before using it we should check if the extracted instruction is existing and still a COPY instruction.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: None (weiguozhi)

Changes

Function foldRedundantCopy records COPY instructions in CopyMIs and uses it later. But other optimizations may delete or modify it. So before using it we should check if the extracted instruction is existing and still a COPY instruction.

---

Full diff: https://github.com/llvm/llvm-project/pull/69511.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/PeepholeOptimizer.cpp (+3-1)
• (added) llvm/test/CodeGen/X86/peephole-copy.ll (+20)

diff --git a/llvm/lib/CodeGen/PeepholeOptimizer.cpp b/llvm/lib/CodeGen/PeepholeOptimizer.cpp
index f413ca5b04f48cf..09ae2680f5516ba 100644
--- a/llvm/lib/CodeGen/PeepholeOptimizer.cpp
+++ b/llvm/lib/CodeGen/PeepholeOptimizer.cpp
@@ -1445,7 +1445,9 @@ bool PeepholeOptimizer::foldRedundantCopy(
   }
 
   MachineInstr *PrevCopy = CopyMIs.find(SrcPair)->second;
-  if (!LocalMIs.count(PrevCopy))
+  // A COPY instruction can be deleted or changed by other optimizations.
+  // Check if the previous COPY instruction is existing and still a COPY.
+  if (!LocalMIs.count(PrevCopy) || !PrevCopy->isCopy())
     return false;
 
   assert(SrcSubReg == PrevCopy->getOperand(1).getSubReg() &&
diff --git a/llvm/test/CodeGen/X86/peephole-copy.ll b/llvm/test/CodeGen/X86/peephole-copy.ll
new file mode 100644
index 000000000000000..60f25a3bc7feee4
--- /dev/null
+++ b/llvm/test/CodeGen/X86/peephole-copy.ll
@@ -0,0 +1,20 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 3
+; RUN: llc < %s -mtriple=i686-- | FileCheck %s
+
+; In peephole optimization the modified COPY instruction should not cause
+; compiler failure.
+
+define dso_local void @c() {
+; CHECK-LABEL: c:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    movl $512, %eax # imm = 0x200
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    retl
+entry:
+  tail call void asm sideeffect "", "q,~{dirflag},~{fpsr},~{flags}"(i32 512)
+  tail call void asm sideeffect "", "q,~{dirflag},~{fpsr},~{flags}"(i32 512)
+  ret void
+}

[mikaelholmen]

I have no idea if this is the way to solve the problem, but I've verified that this patch fixes the problem I saw in
https://reviews.llvm.org/D151848#4654329

Thanks!

[qcolombet]

Could you make this only a peephole test using .mir?

The description mentions a COPY but from the IR it is unclear where it comes from and what happens to it in the peephole optimizer.

[weiguozhi]

done.

[qcolombet]

Here we already stretch the use of the LocalMIs structure to know if something has been changed and now i feel we abuse the CopyMIs structure since it can hold (stale) reference to non copy.

The patch itself is fine as a quick fix, but maybe we'll want to revisit to properly clean up CopyMIs (using the delegate mechanism for instance) and don't require any of these two checks here:

• What is in CopyMIs should not have been deleted or should not be a non-copy.

[qcolombet]

Note: If it is okay to wait a bit longer for the fix, please implement the delegate instead.

[weiguozhi]

To make it clear, if I use the delegate method, I will use the MachineFunction::Delegate. It already has a function MF_HandleRemoval which is useful for our purpose.

// add a new callback function in MachineFunction::Delegate
virtual void MF_HandleChangeDesc(MachineInstr &MI, const MCInstrDesc &TID)


void MachineFunction::handleChangeDesc(MachineInstr &MI, const MCInstrDesc &TID) {
  if (TheDelegate)
    TheDelegate->MF_HandleChangeDesc(MI, TID);
}

// Call the callback when an MachineInstr is modified.
void MachineInstr::setDesc(const MCInstrDesc &TID) {
  getMF()->handleChangeDesc(*this, TID)
   MCID = &TID; 
}

Then we can implement MachineFunction::Delegate in class PeepholeOptimizer, and maintains CopyMIs in the callback functions.

Is this OK?

[qcolombet]

Yep, that's exactly what I had in mind

[weiguozhi]

Thanks for the confirmation.

[weiguozhi]

@mstorsjo, @mikaelholmen, is it urgent for you to fix the assertion?

[mikaelholmen]

@mstorsjo, @mikaelholmen, is it urgent for you to fix the assertion?

Well the problem is a recent regression. I'd appreciate a quick fix, so if you plan to make larger changes taking time I would prefer a fix first.
At the moment I've reverted 760e7d0 downstream since we have tests that fail all the time with that commit. This works for us at the moment but living with downstream reverts always has the potential to create even more work in case something else happens upstream that depends on the reverted patch.
I could of course instead apply this fix downstream, but then again, since it's a recent regression and we've identified the commit that causes it, why shouldn't it be fixed as quickly as possibly on trunk. Either by reverting or fixing.

[mstorsjo]

@mstorsjo, @mikaelholmen, is it urgent for you to fix the assertion?

The failed assertion breaks some of my builds, and unless we have a fix soon I would suggest we revert the offending commit - which I believe is what our developer policy prescribes anyway.

[weiguozhi]

@mstorsjo, @mikaelholmen, is it urgent for you to fix the assertion?

Well the problem is a recent regression. I'd appreciate a quick fix, so if you plan to make larger changes taking time I would prefer a fix first. At the moment I've reverted 760e7d0 downstream since we have tests that fail all the time with that commit. This works for us at the moment but living with downstream reverts always has the potential to create even more work in case something else happens upstream that depends on the reverted patch. I could of course instead apply this fix downstream, but then again, since it's a recent regression and we've identified the commit that causes it, why shouldn't it be fixed as quickly as possibly on trunk. Either by reverting or fixing.

OK, this quick fix is committed.