-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Reland "[LoongArch] Add isSafeToMove hook to prevent unsafe instruction motion"
#167465
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.
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
Conversation
…tion motion" This patch introduces a new virtual method `TargetInstrInfo::isSafeToMove()` to allow backends to control whether a machine instruction can be safely moved by optimization passes. The `BranchFolder` pass now respects this hook when hoisting common code. By default, all instructions are considered safe to to move. For LoongArch, `isSafeToMove()` is overridden to prevent relocation-related instruction sequences (e.g. PC-relative addressing and calls) from being broken by instruction motion. Correspondingly, `isSchedulingBoundary()` is updated to reuse this logic for consistency.
|
@llvm/pr-subscribers-backend-loongarch Author: hev (heiher) ChangesThis patch introduces a new virtual method The For LoongArch, Relands #163725 Full diff: https://github.com/llvm/llvm-project/pull/167465.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 43f28ed79f9dd..18142c2c0adf3 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -1765,6 +1765,17 @@ class LLVM_ABI TargetInstrInfo : public MCInstrInfo {
return true;
}
+ /// Return true if it's safe to move a machine instruction.
+ /// This allows the backend to prevent certain special instruction
+ /// sequences from being broken by instruction motion in optimization
+ /// passes.
+ /// By default, this returns true for every instruction.
+ virtual bool isSafeToMove(const MachineInstr &MI,
+ const MachineBasicBlock *MBB,
+ const MachineFunction &MF) const {
+ return true;
+ }
+
/// Test if the given instruction should be considered a scheduling boundary.
/// This primarily includes labels and terminators.
virtual bool isSchedulingBoundary(const MachineInstr &MI,
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index 7292bc2be0df2..0b212fb0beb20 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -1979,6 +1979,7 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
MachineBasicBlock::iterator FIB = FBB->begin();
MachineBasicBlock::iterator TIE = TBB->end();
MachineBasicBlock::iterator FIE = FBB->end();
+ MachineFunction &MF = *TBB->getParent();
while (TIB != TIE && FIB != FIE) {
// Skip dbg_value instructions. These do not count.
TIB = skipDebugInstructionsForward(TIB, TIE, false);
@@ -1993,6 +1994,10 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
// Hard to reason about register liveness with predicated instruction.
break;
+ if (!TII->isSafeToMove(*TIB, TBB, MF))
+ // Don't hoist the instruction if it isn't safe to move.
+ break;
+
bool IsSafe = true;
for (MachineOperand &MO : TIB->operands()) {
// Don't attempt to hoist instructions with register masks.
diff --git a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp
index 9a33dccd002c7..9fc862af7ea24 100644
--- a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp
+++ b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp
@@ -378,12 +378,9 @@ bool LoongArchInstrInfo::isBranchOffsetInRange(unsigned BranchOp,
}
}
-bool LoongArchInstrInfo::isSchedulingBoundary(const MachineInstr &MI,
- const MachineBasicBlock *MBB,
- const MachineFunction &MF) const {
- if (TargetInstrInfo::isSchedulingBoundary(MI, MBB, MF))
- return true;
-
+bool LoongArchInstrInfo::isSafeToMove(const MachineInstr &MI,
+ const MachineBasicBlock *MBB,
+ const MachineFunction &MF) const {
auto MII = MI.getIterator();
auto MIE = MBB->end();
@@ -429,25 +426,25 @@ bool LoongArchInstrInfo::isSchedulingBoundary(const MachineInstr &MI,
auto MO2 = Lu32I->getOperand(2).getTargetFlags();
if (MO0 == LoongArchII::MO_PCREL_HI && MO1 == LoongArchII::MO_PCREL_LO &&
MO2 == LoongArchII::MO_PCREL64_LO)
- return true;
+ return false;
if ((MO0 == LoongArchII::MO_GOT_PC_HI || MO0 == LoongArchII::MO_LD_PC_HI ||
MO0 == LoongArchII::MO_GD_PC_HI) &&
MO1 == LoongArchII::MO_GOT_PC_LO && MO2 == LoongArchII::MO_GOT_PC64_LO)
- return true;
+ return false;
if (MO0 == LoongArchII::MO_IE_PC_HI && MO1 == LoongArchII::MO_IE_PC_LO &&
MO2 == LoongArchII::MO_IE_PC64_LO)
- return true;
+ return false;
if (MO0 == LoongArchII::MO_DESC_PC_HI &&
MO1 == LoongArchII::MO_DESC_PC_LO &&
MO2 == LoongArchII::MO_DESC64_PC_LO)
- return true;
+ return false;
break;
}
case LoongArch::LU52I_D: {
auto MO = MI.getOperand(2).getTargetFlags();
if (MO == LoongArchII::MO_PCREL64_HI || MO == LoongArchII::MO_GOT_PC64_HI ||
MO == LoongArchII::MO_IE_PC64_HI || MO == LoongArchII::MO_DESC64_PC_HI)
- return true;
+ return false;
break;
}
default:
@@ -487,7 +484,7 @@ bool LoongArchInstrInfo::isSchedulingBoundary(const MachineInstr &MI,
auto MO1 = LoongArchII::getDirectFlags(SecondOp->getOperand(2));
auto MO2 = LoongArchII::getDirectFlags(Ld->getOperand(2));
if (MO1 == LoongArchII::MO_DESC_PC_LO && MO2 == LoongArchII::MO_DESC_LD)
- return true;
+ return false;
break;
}
if (SecondOp == MIE ||
@@ -496,34 +493,34 @@ bool LoongArchInstrInfo::isSchedulingBoundary(const MachineInstr &MI,
auto MO1 = LoongArchII::getDirectFlags(SecondOp->getOperand(2));
if (MO0 == LoongArchII::MO_PCREL_HI && SecondOp->getOpcode() == AddiOp &&
MO1 == LoongArchII::MO_PCREL_LO)
- return true;
+ return false;
if (MO0 == LoongArchII::MO_GOT_PC_HI && SecondOp->getOpcode() == LdOp &&
MO1 == LoongArchII::MO_GOT_PC_LO)
- return true;
+ return false;
if ((MO0 == LoongArchII::MO_LD_PC_HI ||
MO0 == LoongArchII::MO_GD_PC_HI) &&
SecondOp->getOpcode() == AddiOp && MO1 == LoongArchII::MO_GOT_PC_LO)
- return true;
+ return false;
break;
}
case LoongArch::ADDI_W:
case LoongArch::ADDI_D: {
auto MO = LoongArchII::getDirectFlags(MI.getOperand(2));
if (MO == LoongArchII::MO_PCREL_LO || MO == LoongArchII::MO_GOT_PC_LO)
- return true;
+ return false;
break;
}
case LoongArch::LD_W:
case LoongArch::LD_D: {
auto MO = LoongArchII::getDirectFlags(MI.getOperand(2));
if (MO == LoongArchII::MO_GOT_PC_LO)
- return true;
+ return false;
break;
}
case LoongArch::PseudoDESC_CALL: {
auto MO = LoongArchII::getDirectFlags(MI.getOperand(2));
if (MO == LoongArchII::MO_DESC_CALL)
- return true;
+ return false;
break;
}
default:
@@ -531,6 +528,18 @@ bool LoongArchInstrInfo::isSchedulingBoundary(const MachineInstr &MI,
}
}
+ return true;
+}
+
+bool LoongArchInstrInfo::isSchedulingBoundary(const MachineInstr &MI,
+ const MachineBasicBlock *MBB,
+ const MachineFunction &MF) const {
+ if (TargetInstrInfo::isSchedulingBoundary(MI, MBB, MF))
+ return true;
+
+ if (!isSafeToMove(MI, MBB, MF))
+ return true;
+
return false;
}
diff --git a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.h b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.h
index 796ef9f3a5715..9f7a0a2239a87 100644
--- a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.h
+++ b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.h
@@ -66,6 +66,9 @@ class LoongArchInstrInfo : public LoongArchGenInstrInfo {
bool isBranchOffsetInRange(unsigned BranchOpc,
int64_t BrOffset) const override;
+ bool isSafeToMove(const MachineInstr &MI, const MachineBasicBlock *MBB,
+ const MachineFunction &MF) const override;
+
bool isSchedulingBoundary(const MachineInstr &MI,
const MachineBasicBlock *MBB,
const MachineFunction &MF) const override;
|
| // Hard to reason about register liveness with predicated instruction. | ||
| break; | ||
|
|
||
| if (!TII->isSafeToMove(*TIB, TBB, MF)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: MBB -> TBB
|
Could you add #163681 as a testcase? |
Done. Thanks! |
SixWeining
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…tion motion" (llvm#167465) This patch introduces a new virtual method `TargetInstrInfo::isSafeToMove()` to allow backends to control whether a machine instruction can be safely moved by optimization passes. The `BranchFolder` pass now respects this hook when hoisting common code. By default, all instructions are considered safe to to move. For LoongArch, `isSafeToMove()` is overridden to prevent relocation-related instruction sequences (e.g. PC-relative addressing and calls) from being broken by instruction motion. Correspondingly, `isSchedulingBoundary()` is updated to reuse this logic for consistency. Relands llvm#163725 (cherry picked from commit ea10026)
…tion motion" (llvm#167465) This patch introduces a new virtual method `TargetInstrInfo::isSafeToMove()` to allow backends to control whether a machine instruction can be safely moved by optimization passes. The `BranchFolder` pass now respects this hook when hoisting common code. By default, all instructions are considered safe to to move. For LoongArch, `isSafeToMove()` is overridden to prevent relocation-related instruction sequences (e.g. PC-relative addressing and calls) from being broken by instruction motion. Correspondingly, `isSchedulingBoundary()` is updated to reuse this logic for consistency. Relands llvm#163725 (cherry picked from commit ea10026)
This patch introduces a new virtual method
TargetInstrInfo::isSafeToMove()to allow backends to control whether a machine instruction can be safely moved by optimization passes.The
BranchFolderpass now respects this hook when hoisting common code. By default, all instructions are considered safe to to move.For LoongArch,
isSafeToMove()is overridden to prevent relocation-related instruction sequences (e.g. PC-relative addressing and calls) from being broken by instruction motion. Correspondingly,isSchedulingBoundary()is updated to reuse this logic for consistency.Relands #163725