PHIElimination: add target hook to control reuse. #163604
Conversation
|
@llvm/pr-subscribers-llvm-regalloc Author: Junjie Gu (jgu222) ChangesAdd PHI reuse hook so that a target can decide if temporary register during PHI node elimination can be reused. By default, two phis are allowed to use the same temporary register if their right-hand-sides are the same. However, the left-hand sides of phis need to be checked as well for some targets, such as a GPU target. If the register banks of phis's left-hand sides are diffrerent, reuse is not allowed. Thus, this change adds a target hook for this kind of checking as it is target-dependent. This change has no functional change. This is to resolve the issue: Full diff: https://github.com/llvm/llvm-project/pull/163604.diff 2 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 175f205328361..041958b7a3649 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -2158,7 +2158,7 @@ class LLVM_ABI TargetInstrInfo : public MCInstrInfo {
return TargetOpcode::COPY;
}
- /// During PHI eleimination lets target to make necessary checks and
+ /// During PHI elimination lets target to make necessary checks and
/// insert the copy to the PHI destination register in a target specific
/// manner.
virtual MachineInstr *createPHIDestinationCopy(
@@ -2168,7 +2168,7 @@ class LLVM_ABI TargetInstrInfo : public MCInstrInfo {
.addReg(Src);
}
- /// During PHI eleimination lets target to make necessary checks and
+ /// During PHI elimination lets target to make necessary checks and
/// insert the copy to the PHI destination register in a target specific
/// manner.
virtual MachineInstr *createPHISourceCopy(MachineBasicBlock &MBB,
@@ -2180,6 +2180,17 @@ class LLVM_ABI TargetInstrInfo : public MCInstrInfo {
.addReg(Src, 0, SrcSubReg);
}
+ /// During PHI elimination lets target to decide if two phis can use the
+ /// same register \p Reg when they have the same rhs. Register \p Reg has
+ /// been used for the first phi and \p PHIReg is the DestReg of the second
+ /// Phi. This function is to check if the second phi can reuse \p Reg as
+ /// its temporary register.
+ /// The default is to allow reuse.
+ virtual bool allowPHIReuse(Register Reg, Register PHIReg,
+ const MachineFunction &MF) const {
+ return true;
+ }
+
/// Returns a \p outliner::OutlinedFunction struct containing target-specific
/// information for a set of outlining candidates. Returns std::nullopt if the
/// candidates are not suitable for outlining. \p MinRepeats is the minimum
diff --git a/llvm/lib/CodeGen/PHIElimination.cpp b/llvm/lib/CodeGen/PHIElimination.cpp
index 34a9d5d0e401f..65423bd686eff 100644
--- a/llvm/lib/CodeGen/PHIElimination.cpp
+++ b/llvm/lib/CodeGen/PHIElimination.cpp
@@ -380,7 +380,7 @@ void PHIEliminationImpl::LowerPHINode(MachineBasicBlock &MBB,
Register *Entry = nullptr;
if (AllEdgesCritical)
Entry = &LoweredPHIs[MPhi];
- if (Entry && *Entry) {
+ if (Entry && *Entry && TII->allowPHIReuse(*Entry, MPhi, MF)) {
// An identical PHI node was already lowered. Reuse the incoming register.
IncomingReg = *Entry;
reusedIncoming = true;
|
Add PHI reuse hook so that a target can decide if temporary register during PHI node elimination can be reused. By default, two phis are allowed to use the same temporary register if their right-hand-sides are the same. However, the left-hand sides of phis need to be checked as well for some targets, such as a GPU target. If the register banks of phis's left-hand sides are diffrerent, reuse is not allowed. Thus, this change adds a target hook for this kind of checking as it is target-dependent. This change has no functional change. This is to resolve the issue: llvm#163500
jgu222
force-pushed
the
phi_reuse_hook
branch
from
3b8e4be to
5345248
Compare
|
@paperchalice @guy-david Could you help review this ? |
|
@arsenm Could you help review it ? |
|
I don't think this is the correct approach, but correct me if I'm mistaken. |
Thought about using register class to check if reuse is okay, but give up as it is really up to a target. It is possible that two rhs-identical PHIs have their lhs in different register class, and those two different rc could be interchangeable in one target, but not allowed in another. The correctness here is up to how convergence is enforced for a gpu target. When favoring backedge for a loop, this exposes this issue. I feel this is better to be decided by a target. |
|
To me, the phi node elimination assumes that two phis that have identical right-hand sides can use the same virtual temp. This is true for cpu, but not true for gpu due to gpu code's complicate divergence/convergence scheme. If not this one, what approach is more appropriate ? Thanks |
This is a modeling issue |
Kind of agree that this is too specific. Using register class works for me, although I feel this checking is a stronger condition. But I can live with it. How is the following change ? |
|
Is this the problem solved by SIOptimizeVGPRLiveRange? You generally want to work in terms of common subclasses than exact class checks |
4 participants
Add PHI reuse hook so that a target can decide if temporary register during PHI node elimination can be reused. By default, two phis are allowed to use the same temporary register if their right-hand-sides are the same.
However, the left-hand sides of phis need to be checked as well for some targets, such as a GPU target. If the register banks of phis's left-hand sides are diffrerent, reuse is not allowed. Thus, this change adds a target hook for this kind of checking as it is target-dependent.
This change has no functional change.
This is to resolve the issue:
#163500