[llvm] d61d591 - [MachineOutliner] Make getOutliningType partially target-independent
via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 9 11:35:16 PST 2023
Author: duk
Date: 2023-02-09T14:35:00-05:00
New Revision: d61d591411da35414dabd2402130500d06c2fa38
URL: https://github.com/llvm/llvm-project/commit/d61d591411da35414dabd2402130500d06c2fa38
DIFF: https://github.com/llvm/llvm-project/commit/d61d591411da35414dabd2402130500d06c2fa38.diff
LOG: [MachineOutliner] Make getOutliningType partially target-independent
The motivation behind this patch is to unify some of the outliner logic across architectures. This looks nicer in general and makes fixing [issues like this](https://reviews.llvm.org/D124707#3483805) easier.
There are some notable changes here:
1. `isMetaInstruction()` is used directly instead of checking for specific meta-instructions like `IMPLICIT_DEF` or `KILL`. This was already done in the RISC-V implementation, but other architectures still did hardcoded checks.
- As an exception to this, CFI instructions are explicitly delegated to the target because RISC-V has different handling for those.
2. `isTargetIndex()` checks are replaced with an assert; none of the architectures supported actually use `MO_TargetIndex` at this point in time.
3. `isCFIIndex()` and `isFI()` checks are also replaced with asserts, since these operands should not exist in [any context](https://reviews.llvm.org/D122635#3447214) at this stage in the pipeline.
Reviewed by: paquette
Differential Revision: https://reviews.llvm.org/D125072
Added:
Modified:
llvm/include/llvm/CodeGen/TargetInstrInfo.h
llvm/lib/CodeGen/TargetInstrInfo.cpp
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
llvm/lib/Target/AArch64/AArch64InstrInfo.h
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
llvm/lib/Target/ARM/ARMBaseInstrInfo.h
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
llvm/lib/Target/RISCV/RISCVInstrInfo.h
llvm/lib/Target/X86/X86InstrInfo.cpp
llvm/lib/Target/X86/X86InstrInfo.h
Removed:
################################################################################
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index a19259c0a9557..8185751f56cc7 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -1975,13 +1975,20 @@ class TargetInstrInfo : public MCInstrInfo {
virtual void mergeOutliningCandidateAttributes(
Function &F, std::vector<outliner::Candidate> &Candidates) const;
- /// Returns how or if \p MI should be outlined.
+protected:
+ /// Target-dependent implementation for getOutliningTypeImpl.
virtual outliner::InstrType
- getOutliningType(MachineBasicBlock::iterator &MIT, unsigned Flags) const {
+ getOutliningTypeImpl(MachineBasicBlock::iterator &MIT, unsigned Flags) const {
llvm_unreachable(
- "Target didn't implement TargetInstrInfo::getOutliningType!");
+ "Target didn't implement TargetInstrInfo::getOutliningTypeImpl!");
}
+public:
+ /// Returns how or if \p MIT should be outlined. \p Flags is the
+ /// target-specific information returned by isMBBSafeToOutlineFrom.
+ outliner::InstrType
+ getOutliningType(MachineBasicBlock::iterator &MIT, unsigned Flags) const;
+
/// Optional target hook that returns true if \p MBB is safe to outline from,
/// and returns any target-specific information in \p Flags.
virtual bool isMBBSafeToOutlineFrom(MachineBasicBlock &MBB,
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 0f6cf11ca9d17..a90258980a081 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -1555,6 +1555,72 @@ void TargetInstrInfo::mergeOutliningCandidateAttributes(
F.addFnAttr(Attribute::NoUnwind);
}
+outliner::InstrType TargetInstrInfo::getOutliningType(
+ MachineBasicBlock::iterator &MIT, unsigned Flags) const {
+ MachineInstr &MI = *MIT;
+
+ // NOTE: MI.isMetaInstruction() will match CFI_INSTRUCTION, but some targets
+ // have support for outlining those. Special-case that here.
+ if (MI.isCFIInstruction())
+ // Just go right to the target implementation.
+ return getOutliningTypeImpl(MIT, Flags);
+
+ // Don't allow instructions that don't materialize to impact analysis.
+ if (MI.isMetaInstruction())
+ return outliner::InstrType::Invisible;
+
+ // Be conservative about inline assembly.
+ if (MI.isInlineAsm())
+ return outliner::InstrType::Illegal;
+
+ // Labels generally can't safely be outlined.
+ if (MI.isLabel())
+ return outliner::InstrType::Illegal;
+
+ // Is this a terminator for a basic block?
+ if (MI.isTerminator()) {
+ // If this is a branch to another block, we can't outline it.
+ if (!MI.getParent()->succ_empty())
+ return outliner::InstrType::Illegal;
+
+ // Don't outline if the branch is not unconditional.
+ if (isPredicated(MI))
+ return outliner::InstrType::Illegal;
+ }
+
+ // Make sure none of the operands of this instruction do anything that
+ // might break if they're moved outside their current function.
+ // This includes MachineBasicBlock references, BlockAddressses,
+ // Constant pool indices and jump table indices.
+ //
+ // A quick note on MO_TargetIndex:
+ // This doesn't seem to be used in any of the architectures that the
+ // MachineOutliner supports, but it was still filtered out in all of them.
+ // There was one exception (RISC-V), but MO_TargetIndex also isn't used there.
+ // As such, this check is removed both here and in the target-specific
+ // implementations. Instead, we assert to make sure this doesn't
+ // catch anyone off-guard somewhere down the line.
+ for (const MachineOperand &MOP : MI.operands()) {
+ // If you hit this assertion, please remove it and adjust
+ // `getOutliningTypeImpl` for your target appropriately if necessary.
+ // Adding the assertion back to other supported architectures
+ // would be nice too :)
+ assert(!MOP.isTargetIndex() && "This isn't used quite yet!");
+
+ // CFI instructions should already have been filtered out at this point.
+ assert(!MOP.isCFIIndex() && "CFI instructions handled elsewhere!");
+
+ // PrologEpilogInserter should've already run at this point.
+ assert(!MOP.isFI() && "FrameIndex instructions should be gone by now!");
+
+ if (MOP.isMBB() || MOP.isBlockAddress() || MOP.isCPI() || MOP.isJTI())
+ return outliner::InstrType::Illegal;
+ }
+
+ // If we don't know, delegate to the target-specific hook.
+ return getOutliningTypeImpl(MIT, Flags);
+}
+
bool TargetInstrInfo::isMBBSafeToOutlineFrom(MachineBasicBlock &MBB,
unsigned &Flags) const {
// Some instrumentations create special TargetOpcode at the start which
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 0b33d7c7dac85..6a1617910b5f9 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -7695,7 +7695,7 @@ AArch64InstrInfo::getOutlinableRanges(MachineBasicBlock &MBB,
}
outliner::InstrType
-AArch64InstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT,
+AArch64InstrInfo::getOutliningTypeImpl(MachineBasicBlock::iterator &MIT,
unsigned Flags) const {
MachineInstr &MI = *MIT;
MachineBasicBlock *MBB = MI.getParent();
@@ -7728,31 +7728,17 @@ AArch64InstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT,
if (MI.isCFIInstruction())
return outliner::InstrType::Legal;
- // Don't allow debug values to impact outlining type.
- if (MI.isDebugInstr() || MI.isIndirectDebugValue())
- return outliner::InstrType::Invisible;
-
- // At this point, KILL instructions don't really tell us much so we can go
- // ahead and skip over them.
- if (MI.isKill())
- return outliner::InstrType::Invisible;
-
// Is this a terminator for a basic block?
- if (MI.isTerminator()) {
-
- // Is this the end of a function?
- if (MI.getParent()->succ_empty())
- return outliner::InstrType::Legal;
-
- // It's not, so don't outline it.
- return outliner::InstrType::Illegal;
- }
+ if (MI.isTerminator())
+ // TargetInstrInfo::getOutliningType has already filtered out anything
+ // that would break this, so we can allow it here.
+ return outliner::InstrType::Legal;
// Make sure none of the operands are un-outlinable.
for (const MachineOperand &MOP : MI.operands()) {
- if (MOP.isCPI() || MOP.isJTI() || MOP.isCFIIndex() || MOP.isFI() ||
- MOP.isTargetIndex())
- return outliner::InstrType::Illegal;
+ // A check preventing CFI indices was here before, but only CFI
+ // instructions should have those.
+ assert(!MOP.isCFIIndex());
// If it uses LR or W30 explicitly, then don't touch it.
if (MOP.isReg() && !MOP.isImplicit() &&
@@ -7828,10 +7814,6 @@ AArch64InstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT,
return outliner::InstrType::Legal;
}
- // Don't outline positions.
- if (MI.isPosition())
- return outliner::InstrType::Illegal;
-
// Don't touch the link register or W30.
if (MI.readsRegister(AArch64::W30, &getRegisterInfo()) ||
MI.modifiesRegister(AArch64::W30, &getRegisterInfo()))
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index 5b2fb038423a6..96ec0552ac795 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -291,8 +291,8 @@ class AArch64InstrInfo final : public AArch64GenInstrInfo {
bool OutlineFromLinkOnceODRs) const override;
outliner::OutlinedFunction getOutliningCandidateInfo(
std::vector<outliner::Candidate> &RepeatedSequenceLocs) const override;
- outliner::InstrType getOutliningType(MachineBasicBlock::iterator &MIT,
- unsigned Flags) const override;
+ outliner::InstrType
+ getOutliningTypeImpl(MachineBasicBlock::iterator &MIT, unsigned Flags) const override;
SmallVector<
std::pair<MachineBasicBlock::iterator, MachineBasicBlock::iterator>>
getOutlinableRanges(MachineBasicBlock &MBB, unsigned &Flags) const override;
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
index 1bf0b30da24d6..f1ba0154b274f 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -6275,24 +6275,11 @@ bool ARMBaseInstrInfo::isMBBSafeToOutlineFrom(MachineBasicBlock &MBB,
}
outliner::InstrType
-ARMBaseInstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT,
+ARMBaseInstrInfo::getOutliningTypeImpl(MachineBasicBlock::iterator &MIT,
unsigned Flags) const {
MachineInstr &MI = *MIT;
const TargetRegisterInfo *TRI = &getRegisterInfo();
- // Be conservative with inline ASM
- if (MI.isInlineAsm())
- return outliner::InstrType::Illegal;
-
- // Don't allow debug values to impact outlining type.
- if (MI.isDebugInstr() || MI.isIndirectDebugValue())
- return outliner::InstrType::Invisible;
-
- // At this point, KILL or IMPLICIT_DEF instructions don't really tell us much
- // so we can go ahead and skip over them.
- if (MI.isKill() || MI.isImplicitDef())
- return outliner::InstrType::Invisible;
-
// PIC instructions contain labels, outlining them would break offset
// computing. unsigned Opc = MI.getOpcode();
unsigned Opc = MI.getOpcode();
@@ -6318,25 +6305,10 @@ ARMBaseInstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT,
return outliner::InstrType::Illegal;
// Is this a terminator for a basic block?
- if (MI.isTerminator()) {
- // Don't outline if the branch is not unconditional.
- if (isPredicated(MI))
- return outliner::InstrType::Illegal;
-
- // Is this the end of a function?
- if (MI.getParent()->succ_empty())
- return outliner::InstrType::Legal;
-
- // It's not, so don't outline it.
- return outliner::InstrType::Illegal;
- }
-
- // Make sure none of the operands are un-outlinable.
- for (const MachineOperand &MOP : MI.operands()) {
- if (MOP.isCPI() || MOP.isJTI() || MOP.isCFIIndex() || MOP.isFI() ||
- MOP.isTargetIndex())
- return outliner::InstrType::Illegal;
- }
+ if (MI.isTerminator())
+ // TargetInstrInfo::getOutliningType has already filtered out anything
+ // that would break this, so we can allow it here.
+ return outliner::InstrType::Legal;
// Don't outline if link register or program counter value are used.
if (MI.readsRegister(ARM::LR, TRI) || MI.readsRegister(ARM::PC, TRI))
@@ -6441,8 +6413,8 @@ ARMBaseInstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT,
MI.modifiesRegister(ARM::ITSTATE, TRI))
return outliner::InstrType::Illegal;
- // Don't outline positions.
- if (MI.isPosition())
+ // Don't outline CFI instructions.
+ if (MI.isCFIInstruction())
return outliner::InstrType::Illegal;
return outliner::InstrType::Legal;
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.h b/llvm/lib/Target/ARM/ARMBaseInstrInfo.h
index aa9f9c4db1721..70c3be2b54f16 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.h
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.h
@@ -352,7 +352,7 @@ class ARMBaseInstrInfo : public ARMGenInstrInfo {
std::vector<outliner::Candidate> &RepeatedSequenceLocs) const override;
void mergeOutliningCandidateAttributes(
Function &F, std::vector<outliner::Candidate> &Candidates) const override;
- outliner::InstrType getOutliningType(MachineBasicBlock::iterator &MIT,
+ outliner::InstrType getOutliningTypeImpl(MachineBasicBlock::iterator &MIT,
unsigned Flags) const override;
bool isMBBSafeToOutlineFrom(MachineBasicBlock &MBB,
unsigned &Flags) const override;
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index a343fa121836a..83978332a3a86 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -1906,7 +1906,7 @@ outliner::OutlinedFunction RISCVInstrInfo::getOutliningCandidateInfo(
}
outliner::InstrType
-RISCVInstrInfo::getOutliningType(MachineBasicBlock::iterator &MBBI,
+RISCVInstrInfo::getOutliningTypeImpl(MachineBasicBlock::iterator &MBBI,
unsigned Flags) const {
MachineInstr &MI = *MBBI;
MachineBasicBlock *MBB = MI.getParent();
@@ -1914,26 +1914,13 @@ RISCVInstrInfo::getOutliningType(MachineBasicBlock::iterator &MBBI,
MBB->getParent()->getSubtarget().getRegisterInfo();
const auto &F = MI.getMF()->getFunction();
- // Positions generally can't safely be outlined.
- if (MI.isPosition()) {
- // We can manually strip out CFI instructions later.
- if (MI.isCFIInstruction())
- // If current function has exception handling code, we can't outline &
- // strip these CFI instructions since it may break .eh_frame section
- // needed in unwinding.
- return F.needsUnwindTableEntry() ? outliner::InstrType::Illegal
- : outliner::InstrType::Invisible;
-
- return outliner::InstrType::Illegal;
- }
-
- // Don't trust the user to write safe inline assembly.
- if (MI.isInlineAsm())
- return outliner::InstrType::Illegal;
-
- // We can't outline branches to other basic blocks.
- if (MI.isTerminator() && !MBB->succ_empty())
- return outliner::InstrType::Illegal;
+ // We can manually strip out CFI instructions later.
+ if (MI.isCFIInstruction())
+ // If current function has exception handling code, we can't outline &
+ // strip these CFI instructions since it may break .eh_frame section
+ // needed in unwinding.
+ return F.needsUnwindTableEntry() ? outliner::InstrType::Illegal
+ : outliner::InstrType::Invisible;
// We need support for tail calls to outlined functions before return
// statements can be allowed.
@@ -1948,8 +1935,6 @@ RISCVInstrInfo::getOutliningType(MachineBasicBlock::iterator &MBBI,
// Make sure the operands don't reference something unsafe.
for (const auto &MO : MI.operands()) {
- if (MO.isMBB() || MO.isBlockAddress() || MO.isCPI() || MO.isJTI())
- return outliner::InstrType::Illegal;
// pcrel-hi and pcrel-lo can't put in separate sections, filter that out
// if any possible.
@@ -1959,11 +1944,6 @@ RISCVInstrInfo::getOutliningType(MachineBasicBlock::iterator &MBBI,
return outliner::InstrType::Illegal;
}
- // Don't allow instructions which won't be materialized to impact outlining
- // analysis.
- if (MI.isMetaInstruction())
- return outliner::InstrType::Invisible;
-
return outliner::InstrType::Legal;
}
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.h b/llvm/lib/Target/RISCV/RISCVInstrInfo.h
index c663af75a5579..e2e5c424cb3a3 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.h
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.h
@@ -159,8 +159,9 @@ class RISCVInstrInfo : public RISCVGenInstrInfo {
std::vector<outliner::Candidate> &RepeatedSequenceLocs) const override;
// Return if/how a given MachineInstr should be outlined.
- outliner::InstrType getOutliningType(MachineBasicBlock::iterator &MBBI,
- unsigned Flags) const override;
+ virtual outliner::InstrType
+ getOutliningTypeImpl(MachineBasicBlock::iterator &MBBI,
+ unsigned Flags) const override;
// Insert a custom frame for outlined functions.
void buildOutlinedFrame(MachineBasicBlock &MBB, MachineFunction &MF,
diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp
index 9d2a51fe07d51..557e18cf596e0 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -9678,32 +9678,15 @@ bool X86InstrInfo::isFunctionSafeToOutlineFrom(MachineFunction &MF,
}
outliner::InstrType
-X86InstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT, unsigned Flags) const {
+X86InstrInfo::getOutliningTypeImpl(MachineBasicBlock::iterator &MIT, unsigned Flags) const {
MachineInstr &MI = *MIT;
- // Don't allow debug values to impact outlining type.
- if (MI.isDebugInstr() || MI.isIndirectDebugValue())
- return outliner::InstrType::Invisible;
- // At this point, KILL instructions don't really tell us much so we can go
- // ahead and skip over them.
- if (MI.isKill())
- return outliner::InstrType::Invisible;
-
- // Is this a tail call? If yes, we can outline as a tail call.
- if (isTailCall(MI))
+ // Is this a terminator for a basic block?
+ if (MI.isTerminator())
+ // TargetInstrInfo::getOutliningType has already filtered out anything
+ // that would break this, so we can allow it here.
return outliner::InstrType::Legal;
- // Is this the terminator of a basic block?
- if (MI.isTerminator() || MI.isReturn()) {
-
- // Does its parent have any successors in its MachineFunction?
- if (MI.getParent()->succ_empty())
- return outliner::InstrType::Legal;
-
- // It does, so we can't tail call it.
- return outliner::InstrType::Illegal;
- }
-
// Don't outline anything that modifies or reads from the stack pointer.
//
// FIXME: There are instructions which are being manually built without
@@ -9724,16 +9707,10 @@ X86InstrInfo::getOutliningType(MachineBasicBlock::iterator &MIT, unsigned Flags
MI.getDesc().hasImplicitDefOfPhysReg(X86::RIP))
return outliner::InstrType::Illegal;
- // Positions can't safely be outlined.
- if (MI.isPosition())
+ // Don't outline CFI instructions.
+ if (MI.isCFIInstruction())
return outliner::InstrType::Illegal;
- // Make sure none of the operands of this instruction do anything tricky.
- for (const MachineOperand &MOP : MI.operands())
- if (MOP.isCPI() || MOP.isJTI() || MOP.isCFIIndex() || MOP.isFI() ||
- MOP.isTargetIndex())
- return outliner::InstrType::Illegal;
-
return outliner::InstrType::Legal;
}
diff --git a/llvm/lib/Target/X86/X86InstrInfo.h b/llvm/lib/Target/X86/X86InstrInfo.h
index 02cb5f761747e..ff588b1d78f38 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.h
+++ b/llvm/lib/Target/X86/X86InstrInfo.h
@@ -558,7 +558,7 @@ class X86InstrInfo final : public X86GenInstrInfo {
bool OutlineFromLinkOnceODRs) const override;
outliner::InstrType
- getOutliningType(MachineBasicBlock::iterator &MIT, unsigned Flags) const override;
+ getOutliningTypeImpl(MachineBasicBlock::iterator &MIT, unsigned Flags) const override;
void buildOutlinedFrame(MachineBasicBlock &MBB, MachineFunction &MF,
const outliner::OutlinedFunction &OF) const override;
More information about the llvm-commits
mailing list