Skip to content

[Peephole] Check instructions from CopyMIs are still COPY #69511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 20, 2023
Merged

[Peephole] Check instructions from CopyMIs are still COPY #69511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f477b08
[Peephole] Check instructions from CopyMIs are still COPY
weiguozhi Oct 18, 2023
3ef1cf2
Change the .ll test case to .mir. So the COPY instructions can be
weiguozhi Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion llvm/lib/CodeGen/PeepholeOptimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's exactly what I had in mind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the confirmation.

return false;

assert(SrcSubReg == PrevCopy->getOperand(1).getSubReg() &&
Expand Down
34 changes: 34 additions & 0 deletions llvm/test/CodeGen/X86/peephole-copy.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3
# RUN: llc -mtriple=i686-- -run-pass=peephole-opt %s -o - | FileCheck %s
--- |
define void @c() {
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
}
...
---
# In peephole optimization the modified COPY instruction should not cause
# compiler failure.
name: c
registers:
- { id: 0, class: gr32_abcd }
- { id: 1, class: gr32_abcd }
- { id: 2, class: gr32 }

body: |
bb.0:
; CHECK-LABEL: name: c
; CHECK: [[MOV32ri:%[0-9]+]]:gr32_abcd = MOV32ri 512
; CHECK-NEXT: INLINEASM &"", 1 /* sideeffect attdialect */, 2359305 /* reguse:GR32 */, [[MOV32ri]], 1 /* reguse */, implicit-def early-clobber $df
; CHECK-NEXT: [[MOV32ri1:%[0-9]+]]:gr32_abcd = MOV32ri 512
; CHECK-NEXT: INLINEASM &"", 1 /* sideeffect attdialect */, 2359305 /* reguse:GR32 */, [[MOV32ri1]], 1 /* reguse */, implicit-def early-clobber $df
; CHECK-NEXT: RET 0
%2 = MOV32ri 512
%0 = COPY %2
INLINEASM &"", 1 /* sideeffect attdialect */, 2359305 /* reguse:GR32_ABCD */, %0:gr32_abcd, 1 /* clobber */, implicit-def early-clobber $df
%1 = COPY %2
INLINEASM &"", 1 /* sideeffect attdialect */, 2359305 /* reguse:GR32_ABCD */, %1:gr32_abcd, 1 /* clobber */, implicit-def early-clobber $df
RET 0
...