Skip to content

[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead #105956

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 4 commits into from
Sep 9, 2024
Merged

[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead #105956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e19d7be
[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead
tobias-stadler Aug 24, 2024
40d27dc
Rename function
tobias-stadler Aug 27, 2024
6aaf161
Return expression
tobias-stadler Aug 28, 2024
aff8bd2
phi check first
tobias-stadler Aug 29, 2024
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
9 changes: 9 additions & 0 deletions llvm/include/llvm/CodeGen/MachineInstr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,6 +1323,11 @@ class MachineInstr
return getOpcode() == TargetOpcode::ANNOTATION_LABEL;
}

bool isLifetimeMarker() const {
return getOpcode() == TargetOpcode::LIFETIME_START ||
getOpcode() == TargetOpcode::LIFETIME_END;
}

/// Returns true if the MachineInstr represents a label.
bool isLabel() const {
return isEHLabel() || isGCLabel() || isAnnotationLabel();
Expand Down Expand Up @@ -1727,6 +1732,10 @@ class MachineInstr
/// the instruction's location and its intended destination.
bool isSafeToMove(bool &SawStore) const;

/// Return true if this instruction would be trivially dead if all of its
/// defined registers were dead.
bool wouldBeTriviallyDead() const;

/// Returns true if this instruction's memory access aliases the memory
/// access of Other.
//
Expand Down
34 changes: 15 additions & 19 deletions llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,23 +80,9 @@ INITIALIZE_PASS(DeadMachineInstructionElim, DEBUG_TYPE,
"Remove dead machine instructions", false, false)

bool DeadMachineInstructionElimImpl::isDead(const MachineInstr *MI) const {
// Technically speaking inline asm without side effects and no defs can still
// be deleted. But there is so much bad inline asm code out there, we should
// let them be.
if (MI->isInlineAsm())
return false;

// Don't delete frame allocation labels.
if (MI->getOpcode() == TargetOpcode::LOCAL_ESCAPE ||
MI->getOpcode() == TargetOpcode::FAKE_USE)
return false;

// Don't delete instructions with side effects.
bool SawStore = false;
if (!MI->isSafeToMove(SawStore) && !MI->isPHI())
return false;

// Examine each operand.
// Instructions without side-effects are dead iff they only define dead regs.
// This function is hot and this loop returns early in the common case,
// so only perform additional checks before this if absolutely necessary.
for (const MachineOperand &MO : MI->all_defs()) {
Register Reg = MO.getReg();
if (Reg.isPhysical()) {
Expand All @@ -120,8 +106,18 @@ bool DeadMachineInstructionElimImpl::isDead(const MachineInstr *MI) const {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't the isPhysical path consider the dead flag on the operand?

}

// If there are no defs with uses, the instruction is dead.
return true;
// Technically speaking inline asm without side effects and no defs can still
// be deleted. But there is so much bad inline asm code out there, we should
// let them be.
if (MI->isInlineAsm())
return false;

// FIXME: See issue #105950 for why LIFETIME markers are considered dead here.
if (MI->isLifetimeMarker())
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder why lifetime markers are considered safe to move...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like lifetime markers should be considered in MachineInstr::isPosition, which would also make them unsafe to move.
I might try moving the LIFETIME marker and LOCAL_ESCAPE checks in separate commits... to make this easy to revert when it inevitably breaks someone's code :).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like considering isLifetimeMarker here and the opposite down in wouldBeTriviallyDead but I guess you're planning on fixing that

Copy link
Contributor Author

@tobias-stadler tobias-stadler Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I want to fix this properly, but I wanted to keep this NFC except for the compile-time improvements. I think for now it's better to make this sketchy behavior explicit in DeadMIElim rather than silently dropping lifetime markers by pretending they are safe to move, which makes little sense. I want to add lifetime markers to MachineInstr::isPosition, which makes them unsafe to move. This should get rid of the ugly opposite check.

return true;

// If there are no defs with uses, the instruction might be dead.
return MI->wouldBeTriviallyDead();
}

bool DeadMachineInstructionElimImpl::runImpl(MachineFunction &MF) {
Expand Down
27 changes: 4 additions & 23 deletions llvm/lib/CodeGen/GlobalISel/Utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,34 +221,15 @@ bool llvm::canReplaceReg(Register DstReg, Register SrcReg,

bool llvm::isTriviallyDead(const MachineInstr &MI,
const MachineRegisterInfo &MRI) {
// FIXME: This logical is mostly duplicated with
// DeadMachineInstructionElim::isDead. Why is LOCAL_ESCAPE not considered in
// MachineInstr::isLabel?

// Don't delete frame allocation labels.
if (MI.getOpcode() == TargetOpcode::LOCAL_ESCAPE)
return false;
// Don't delete fake uses.
if (MI.getOpcode() == TargetOpcode::FAKE_USE)
return false;
// LIFETIME markers should be preserved even if they seem dead.
if (MI.getOpcode() == TargetOpcode::LIFETIME_START ||
MI.getOpcode() == TargetOpcode::LIFETIME_END)
return false;

// If we can move an instruction, we can remove it. Otherwise, it has
// a side-effect of some sort.
bool SawStore = false;
if (!MI.isSafeToMove(SawStore) && !MI.isPHI())
return false;

// Instructions without side-effects are dead iff they only define dead vregs.
// Instructions without side-effects are dead iff they only define dead regs.
// This function is hot and this loop returns early in the common case,
// so only perform additional checks before this if absolutely necessary.
for (const auto &MO : MI.all_defs()) {
Register Reg = MO.getReg();
if (Reg.isPhysical() || !MRI.use_nodbg_empty(Reg))
return false;
}
return true;
return MI.wouldBeTriviallyDead();
}

static void reportGISelDiagnostic(DiagnosticSeverity Severity,
Expand Down
22 changes: 22 additions & 0 deletions llvm/lib/CodeGen/MachineInstr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,6 +1323,28 @@ bool MachineInstr::isSafeToMove(bool &SawStore) const {
return true;
}

bool MachineInstr::wouldBeTriviallyDead() const {
// Don't delete frame allocation labels.
// FIXME: Why is LOCAL_ESCAPE not considered in MachineInstr::isLabel?
if (getOpcode() == TargetOpcode::LOCAL_ESCAPE)
return false;

// Don't delete FAKE_USE.
// FIXME: Why is FAKE_USE not considered in MachineInstr::isPosition?
if (isFakeUse())
return false;

// LIFETIME markers should be preserved.
// FIXME: Why are LIFETIME markers not considered in MachineInstr::isPosition?
if (isLifetimeMarker())
return false;

// If we can move an instruction, we can remove it. Otherwise, it has
// a side-effect of some sort.
bool SawStore = false;
return isPHI() || isSafeToMove(SawStore);
}

static bool MemOperandsHaveAlias(const MachineFrameInfo &MFI, AAResults *AA,
bool UseTBAA, const MachineMemOperand *MMOa,
const MachineMemOperand *MMOb) {
Expand Down
Loading