[CodeGen] Make the parameter TRI required in some functions. #85968
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
backend:ARM
backend:AArch64
backend:AMDGPU
backend:RISC-V
backend:PowerPC
backend:SystemZ
backend:WebAssembly
backend:X86
llvm:regalloc
llvm:globalisel
backend:m68k
llvm:SelectionDAG
|
@llvm/pr-subscribers-llvm-regalloc @llvm/pr-subscribers-backend-risc-v Author: Xu Zhang (simonzgx) ChangesMake the parameter TRI required in some functions to prevent miscalling. Patch is 127.03 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85968.diff 82 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
index ca62f38061b115..58a200b16ddee1 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
@@ -906,7 +906,7 @@ class LegalizationArtifactCombiner {
unsigned &DefOperandIdx) {
if (Register Def = findValueFromDefImpl(Reg, 0, Size)) {
if (auto *Unmerge = dyn_cast<GUnmerge>(MRI.getVRegDef(Def))) {
- DefOperandIdx = Unmerge->findRegisterDefOperandIdx(Def);
+ DefOperandIdx = Unmerge->findRegisterDefOperandIdx(Def, nullptr);
return Unmerge;
}
}
diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h
index fcdd73d8b65fdd..69604292f5cd1b 100644
--- a/llvm/include/llvm/CodeGen/MachineInstr.h
+++ b/llvm/include/llvm/CodeGen/MachineInstr.h
@@ -1443,9 +1443,8 @@ class MachineInstr
/// is a read of a super-register.
/// This does not count partial redefines of virtual registers as reads:
/// %reg1024:6 = OP.
- bool readsRegister(Register Reg,
- const TargetRegisterInfo *TRI = nullptr) const {
- return findRegisterUseOperandIdx(Reg, false, TRI) != -1;
+ bool readsRegister(Register Reg, const TargetRegisterInfo *TRI) const {
+ return findRegisterUseOperandIdx(Reg, TRI, false) != -1;
}
/// Return true if the MachineInstr reads the specified virtual register.
@@ -1464,34 +1463,30 @@ class MachineInstr
/// Return true if the MachineInstr kills the specified register.
/// If TargetRegisterInfo is non-null, then it also checks if there is
/// a kill of a super-register.
- bool killsRegister(Register Reg,
- const TargetRegisterInfo *TRI = nullptr) const {
- return findRegisterUseOperandIdx(Reg, true, TRI) != -1;
+ bool killsRegister(Register Reg, const TargetRegisterInfo *TRI) const {
+ return findRegisterUseOperandIdx(Reg, TRI, true) != -1;
}
/// Return true if the MachineInstr fully defines the specified register.
/// If TargetRegisterInfo is non-null, then it also checks
/// if there is a def of a super-register.
/// NOTE: It's ignoring subreg indices on virtual registers.
- bool definesRegister(Register Reg,
- const TargetRegisterInfo *TRI = nullptr) const {
- return findRegisterDefOperandIdx(Reg, false, false, TRI) != -1;
+ bool definesRegister(Register Reg, const TargetRegisterInfo *TRI) const {
+ return findRegisterDefOperandIdx(Reg, TRI, false, false) != -1;
}
/// Return true if the MachineInstr modifies (fully define or partially
/// define) the specified register.
/// NOTE: It's ignoring subreg indices on virtual registers.
- bool modifiesRegister(Register Reg,
- const TargetRegisterInfo *TRI = nullptr) const {
- return findRegisterDefOperandIdx(Reg, false, true, TRI) != -1;
+ bool modifiesRegister(Register Reg, const TargetRegisterInfo *TRI) const {
+ return findRegisterDefOperandIdx(Reg, TRI, false, true) != -1;
}
/// Returns true if the register is dead in this machine instruction.
/// If TargetRegisterInfo is non-null, then it also checks
/// if there is a dead def of a super-register.
- bool registerDefIsDead(Register Reg,
- const TargetRegisterInfo *TRI = nullptr) const {
- return findRegisterDefOperandIdx(Reg, true, false, TRI) != -1;
+ bool registerDefIsDead(Register Reg, const TargetRegisterInfo *TRI) const {
+ return findRegisterDefOperandIdx(Reg, TRI, true, false) != -1;
}
/// Returns true if the MachineInstr has an implicit-use operand of exactly
@@ -1501,22 +1496,23 @@ class MachineInstr
/// Returns the operand index that is a use of the specific register or -1
/// if it is not found. It further tightens the search criteria to a use
/// that kills the register if isKill is true.
- int findRegisterUseOperandIdx(Register Reg, bool isKill = false,
- const TargetRegisterInfo *TRI = nullptr) const;
+ int findRegisterUseOperandIdx(Register Reg, const TargetRegisterInfo *TRI,
+ bool isKill = false) const;
/// Wrapper for findRegisterUseOperandIdx, it returns
/// a pointer to the MachineOperand rather than an index.
- MachineOperand *findRegisterUseOperand(Register Reg, bool isKill = false,
- const TargetRegisterInfo *TRI = nullptr) {
- int Idx = findRegisterUseOperandIdx(Reg, isKill, TRI);
+ MachineOperand *findRegisterUseOperand(Register Reg,
+ const TargetRegisterInfo *TRI,
+ bool isKill = false) {
+ int Idx = findRegisterUseOperandIdx(Reg, TRI, isKill);
return (Idx == -1) ? nullptr : &getOperand(Idx);
}
- const MachineOperand *findRegisterUseOperand(
- Register Reg, bool isKill = false,
- const TargetRegisterInfo *TRI = nullptr) const {
- return const_cast<MachineInstr *>(this)->
- findRegisterUseOperand(Reg, isKill, TRI);
+ const MachineOperand *findRegisterUseOperand(Register Reg,
+ const TargetRegisterInfo *TRI,
+ bool isKill = false) const {
+ return const_cast<MachineInstr *>(this)->findRegisterUseOperand(Reg, TRI,
+ isKill);
}
/// Returns the operand index that is a def of the specified register or
@@ -1525,26 +1521,26 @@ class MachineInstr
/// overlap the specified register. If TargetRegisterInfo is non-null,
/// then it also checks if there is a def of a super-register.
/// This may also return a register mask operand when Overlap is true.
- int findRegisterDefOperandIdx(Register Reg,
- bool isDead = false, bool Overlap = false,
- const TargetRegisterInfo *TRI = nullptr) const;
+ int findRegisterDefOperandIdx(Register Reg, const TargetRegisterInfo *TRI,
+ bool isDead = false,
+ bool Overlap = false) const;
/// Wrapper for findRegisterDefOperandIdx, it returns
/// a pointer to the MachineOperand rather than an index.
- MachineOperand *
- findRegisterDefOperand(Register Reg, bool isDead = false,
- bool Overlap = false,
- const TargetRegisterInfo *TRI = nullptr) {
- int Idx = findRegisterDefOperandIdx(Reg, isDead, Overlap, TRI);
+ MachineOperand *findRegisterDefOperand(Register Reg,
+ const TargetRegisterInfo *TRI,
+ bool isDead = false,
+ bool Overlap = false) {
+ int Idx = findRegisterDefOperandIdx(Reg, TRI, isDead, Overlap);
return (Idx == -1) ? nullptr : &getOperand(Idx);
}
- const MachineOperand *
- findRegisterDefOperand(Register Reg, bool isDead = false,
- bool Overlap = false,
- const TargetRegisterInfo *TRI = nullptr) const {
+ const MachineOperand *findRegisterDefOperand(Register Reg,
+ const TargetRegisterInfo *TRI,
+ bool isDead = false,
+ bool Overlap = false) const {
return const_cast<MachineInstr *>(this)->findRegisterDefOperand(
- Reg, isDead, Overlap, TRI);
+ Reg, TRI, isDead, Overlap);
}
/// Find the index of the first operand in the
diff --git a/llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp b/llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp
index ed6ce6bc73d38c..7c219a91acdb8d 100644
--- a/llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp
+++ b/llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp
@@ -231,9 +231,9 @@ bool AggressiveAntiDepBreaker::IsImplicitDefUse(MachineInstr &MI,
MachineOperand *Op = nullptr;
if (MO.isDef())
- Op = MI.findRegisterUseOperand(Reg, true);
+ Op = MI.findRegisterUseOperand(Reg, nullptr, true);
else
- Op = MI.findRegisterDefOperand(Reg);
+ Op = MI.findRegisterDefOperand(Reg, nullptr);
return(Op && Op->isImplicit());
}
@@ -679,7 +679,7 @@ bool AggressiveAntiDepBreaker::FindSuitableFreeRegisters(
// defines 'NewReg' via an early-clobber operand.
for (const auto &Q : make_range(RegRefs.equal_range(Reg))) {
MachineInstr *UseMI = Q.second.Operand->getParent();
- int Idx = UseMI->findRegisterDefOperandIdx(NewReg, false, true, TRI);
+ int Idx = UseMI->findRegisterDefOperandIdx(NewReg, TRI, false, true);
if (Idx == -1)
continue;
@@ -846,7 +846,8 @@ unsigned AggressiveAntiDepBreaker::BreakAntiDependencies(
continue;
} else {
// No anti-dep breaking for implicit deps
- MachineOperand *AntiDepOp = MI.findRegisterDefOperand(AntiDepReg);
+ MachineOperand *AntiDepOp =
+ MI.findRegisterDefOperand(AntiDepReg, nullptr);
assert(AntiDepOp && "Can't find index for defined register operand");
if (!AntiDepOp || AntiDepOp->isImplicit()) {
LLVM_DEBUG(dbgs() << " (implicit)\n");
diff --git a/llvm/lib/CodeGen/CalcSpillWeights.cpp b/llvm/lib/CodeGen/CalcSpillWeights.cpp
index f3cb7fa5af6148..6eebc1fc5ca8ce 100644
--- a/llvm/lib/CodeGen/CalcSpillWeights.cpp
+++ b/llvm/lib/CodeGen/CalcSpillWeights.cpp
@@ -251,7 +251,8 @@ float VirtRegAuxInfo::weightCalcHelper(LiveInterval &LI, SlotIndex *Start,
// For terminators that produce values, ask the backend if the register is
// not spillable.
- if (TII.isUnspillableTerminator(MI) && MI->definesRegister(LI.reg())) {
+ if (TII.isUnspillableTerminator(MI) &&
+ MI->definesRegister(LI.reg(), nullptr)) {
LI.markNotSpillable();
return -1.0f;
}
diff --git a/llvm/lib/CodeGen/CodeGenCommonISel.cpp b/llvm/lib/CodeGen/CodeGenCommonISel.cpp
index 577c5dbc8e2da8..fc56bbe5c5d020 100644
--- a/llvm/lib/CodeGen/CodeGenCommonISel.cpp
+++ b/llvm/lib/CodeGen/CodeGenCommonISel.cpp
@@ -260,7 +260,7 @@ void llvm::salvageDebugInfoForDbgValue(const MachineRegisterInfo &MRI,
continue;
}
- int UseMOIdx = DbgMI->findRegisterUseOperandIdx(DefMO->getReg());
+ int UseMOIdx = DbgMI->findRegisterUseOperandIdx(DefMO->getReg(), nullptr);
assert(UseMOIdx != -1 && DbgMI->hasDebugOperandForReg(DefMO->getReg()) &&
"Must use salvaged instruction as its location");
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index 31e107ade1ccbb..1eecf9e2a54e17 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -599,8 +599,8 @@ static bool hasSameValue(const MachineRegisterInfo &MRI,
return false;
// Further, check that the two defs come from corresponding operands.
- int TIdx = TDef->findRegisterDefOperandIdx(TReg);
- int FIdx = FDef->findRegisterDefOperandIdx(FReg);
+ int TIdx = TDef->findRegisterDefOperandIdx(TReg, nullptr);
+ int FIdx = FDef->findRegisterDefOperandIdx(FReg, nullptr);
if (TIdx == -1 || FIdx == -1)
return false;
diff --git a/llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp b/llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
index 4d668c53f7156b..3bb9da5f1a37bb 100644
--- a/llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
+++ b/llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp
@@ -112,7 +112,7 @@ static Register performCopyPropagation(Register Reg,
bool &IsKill, const TargetInstrInfo &TII,
const TargetRegisterInfo &TRI) {
// First check if statepoint itself uses Reg in non-meta operands.
- int Idx = RI->findRegisterUseOperandIdx(Reg, false, &TRI);
+ int Idx = RI->findRegisterUseOperandIdx(Reg, &TRI, false);
if (Idx >= 0 && (unsigned)Idx < StatepointOpers(&*RI).getNumDeoptArgsIdx()) {
IsKill = false;
return Reg;
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index d3f86af1e2908e..39eb5c42466ee2 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -2749,8 +2749,8 @@ bool CombinerHelper::matchEqualDefs(const MachineOperand &MOP1,
// %5:_(s8), %6:_(s8), %7:_(s8), %8:_(s8) = G_UNMERGE_VALUES %4:_(<4 x s8>)
// I1 and I2 are different instructions but produce same values,
// %1 and %6 are same, %1 and %7 are not the same value.
- return I1->findRegisterDefOperandIdx(InstAndDef1->Reg) ==
- I2->findRegisterDefOperandIdx(InstAndDef2->Reg);
+ return I1->findRegisterDefOperandIdx(InstAndDef1->Reg, nullptr) ==
+ I2->findRegisterDefOperandIdx(InstAndDef2->Reg, nullptr);
}
return false;
}
diff --git a/llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp b/llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
index bb5363fb2527b5..a0967746397f47 100644
--- a/llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
@@ -420,7 +420,8 @@ void RegBankSelect::tryAvoidingSplit(
// If the next terminator uses Reg, this means we have
// to split right after MI and thus we need a way to ask
// which outgoing edges are affected.
- assert(!Next->readsRegister(Reg) && "Need to split between terminators");
+ assert(!Next->readsRegister(Reg, nullptr) &&
+ "Need to split between terminators");
// We will split all the edges and repair there.
} else {
// This is a virtual register defined by a terminator.
diff --git a/llvm/lib/CodeGen/InlineSpiller.cpp b/llvm/lib/CodeGen/InlineSpiller.cpp
index c46b1fe18ca743..01e0adf03dcd1b 100644
--- a/llvm/lib/CodeGen/InlineSpiller.cpp
+++ b/llvm/lib/CodeGen/InlineSpiller.cpp
@@ -869,7 +869,7 @@ static void dumpMachineInstrRangeWithSlotIndex(MachineBasicBlock::iterator B,
// destination that is marked as an early clobber, print the
// early-clobber slot index.
if (VReg) {
- MachineOperand *MO = I->findRegisterDefOperand(VReg);
+ MachineOperand *MO = I->findRegisterDefOperand(VReg, nullptr);
if (MO && MO->isEarlyClobber())
Idx = Idx.getRegSlot(true);
}
diff --git a/llvm/lib/CodeGen/LiveVariables.cpp b/llvm/lib/CodeGen/LiveVariables.cpp
index b85526cfb380b6..10df5fe1aa96c8 100644
--- a/llvm/lib/CodeGen/LiveVariables.cpp
+++ b/llvm/lib/CodeGen/LiveVariables.cpp
@@ -258,7 +258,7 @@ void LiveVariables::HandlePhysRegUse(Register Reg, MachineInstr &MI) {
}
}
} else if (LastDef && !PhysRegUse[Reg] &&
- !LastDef->findRegisterDefOperand(Reg))
+ !LastDef->findRegisterDefOperand(Reg, nullptr))
// Last def defines the super register, add an implicit def of reg.
LastDef->addOperand(MachineOperand::CreateReg(Reg, true/*IsDef*/,
true/*IsImp*/));
@@ -361,7 +361,8 @@ bool LiveVariables::HandlePhysRegKill(Register Reg, MachineInstr *MI) {
continue;
bool NeedDef = true;
if (PhysRegDef[Reg] == PhysRegDef[SubReg]) {
- MachineOperand *MO = PhysRegDef[Reg]->findRegisterDefOperand(SubReg);
+ MachineOperand *MO =
+ PhysRegDef[Reg]->findRegisterDefOperand(SubReg, nullptr);
if (MO) {
NeedDef = false;
assert(!MO->isDead());
@@ -388,7 +389,7 @@ bool LiveVariables::HandlePhysRegKill(Register Reg, MachineInstr *MI) {
true/*IsImp*/, true/*IsKill*/));
else {
MachineOperand *MO =
- LastRefOrPartRef->findRegisterDefOperand(Reg, false, false, TRI);
+ LastRefOrPartRef->findRegisterDefOperand(Reg, TRI, false, false);
bool NeedEC = MO->isEarlyClobber() && MO->getReg() != Reg;
// If the last reference is the last def, then it's not used at all.
// That is, unless we are currently processing the last reference itself.
@@ -396,7 +397,7 @@ bool LiveVariables::HandlePhysRegKill(Register Reg, MachineInstr *MI) {
if (NeedEC) {
// If we are adding a subreg def and the superreg def is marked early
// clobber, add an early clobber marker to the subreg def.
- MO = LastRefOrPartRef->findRegisterDefOperand(Reg);
+ MO = LastRefOrPartRef->findRegisterDefOperand(Reg, nullptr);
if (MO)
MO->setIsEarlyClobber();
}
@@ -727,7 +728,7 @@ void LiveVariables::recomputeForSingleDefVirtReg(Register Reg) {
if (MI.isPHI())
break;
if (MI.readsVirtualRegister(Reg)) {
- assert(!MI.killsRegister(Reg));
+ assert(!MI.killsRegister(Reg, nullptr));
MI.addRegisterKilled(Reg, nullptr);
VI.Kills.push_back(&MI);
break;
diff --git a/llvm/lib/CodeGen/MachineCSE.cpp b/llvm/lib/CodeGen/MachineCSE.cpp
index 26a8d00e662651..42cdcaa5bbf4f2 100644
--- a/llvm/lib/CodeGen/MachineCSE.cpp
+++ b/llvm/lib/CodeGen/MachineCSE.cpp
@@ -709,7 +709,7 @@ bool MachineCSE::ProcessBlockCSE(MachineBasicBlock *MBB) {
for (MachineBasicBlock::iterator II = CSMI, IE = &MI; II != IE; ++II)
for (auto ImplicitDef : ImplicitDefs)
if (MachineOperand *MO = II->findRegisterUseOperand(
- ImplicitDef, /*isKill=*/true, TRI))
+ ImplicitDef, TRI, /*isKill=*/true))
MO->setIsKill(false);
} else {
// If the instructions aren't in the same BB, bail out and clear the
diff --git a/llvm/lib/CodeGen/MachineCombiner.cpp b/llvm/lib/CodeGen/MachineCombiner.cpp
index a4c87a7678bd8d..3a50a17a0ebcfa 100644
--- a/llvm/lib/CodeGen/MachineCombiner.cpp
+++ b/llvm/lib/CodeGen/MachineCombiner.cpp
@@ -229,8 +229,8 @@ MachineCombiner::getDepth(SmallVectorImpl<MachineInstr *> &InsInstrs,
assert(DefInstr &&
"There must be a definition for a new virtual register");
DepthOp = InstrDepth[II->second];
- int DefIdx = DefInstr->findRegisterDefOperandIdx(MO.getReg());
- int UseIdx = InstrPtr->findRegisterUseOperandIdx(MO.getReg());
+ int DefIdx = DefInstr->findRegisterDefOperandIdx(MO.getReg(), nullptr);
+ int UseIdx = InstrPtr->findRegisterUseOperandIdx(MO.getReg(), nullptr);
LatencyOp = TSchedModel.computeOperandLatency(DefInstr, DefIdx,
InstrPtr, UseIdx);
} else {
@@ -241,8 +241,10 @@ MachineCombiner::getDepth(SmallVectorImpl<MachineInstr *> &InsInstrs,
DepthOp = BlockTrace.getInstrCycles(*DefInstr).Depth;
if (!isTransientMI(DefInstr))
LatencyOp = TSchedModel.computeOperandLatency(
- DefInstr, DefInstr->findRegisterDefOperandIdx(MO.getReg()),
- InstrPtr, InstrPtr->findRegisterUseOperandIdx(MO.getReg()));
+ DefInstr,
+ DefInstr->findRegisterDefOperandIdx(MO.getReg(), nullptr),
+ InstrPtr,
+ InstrPtr->findRegisterUseOperandIdx(MO.getReg(), nullptr));
}
}
IDepth = std::max(IDepth, DepthOp + LatencyOp);
@@ -280,8 +282,8 @@ unsigned MachineCombiner::getLatency(MachineInstr *Root, MachineInstr *NewRoot,
unsigned LatencyOp = 0;
if (UseMO && BlockTrace.isDepInTrace(*Root, *UseMO)) {
LatencyOp = TSchedModel.computeOperandLatency(
- NewRoot, NewRoot->findRegisterDefOperandIdx(MO.getReg()), UseMO,
- UseMO->findRegisterUseOperandIdx(MO.getReg()));
+ NewRoot, NewRoot->findRegisterDefOperandIdx(MO.getReg(), nullptr),
+ UseMO, UseMO->findRegisterUseOperandIdx(MO.getReg(), nullptr));
} else {
LatencyOp = TSchedModel.computeInstrLatency(NewRoot);
}
diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 9a0ab300b21b7a..30eea0d09ef628 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -720,7 +720,7 @@ void MachineCopyPropagation::forwardUses(MachineInstr &MI) {
// cannot cope with that.
if (isCopyInstr(MI, *TII, UseCopyInstr) &&
MI.modifiesRegister(Copy...
[truncated]
|
|
Are there any functional changes in this patch or are we just passing nullptr wherever we used to use the default? |
|
This is the NFC refactor stage, @AtariDreams is looking at missing TRI entries on #82411 but that has currently stalled due to missing test coverage. |
That was my expectation, but its not stated explicitly in the description and its a large patch so hard to quickly spot any changes. |
|
What's the plan here - are you going to enforce that all callers pass in a non-null TRI? |
|
Can you refine the description to say it's the various register liveness queries on MachineInstr? I think the default parameter should stay as-is. We should assert Reg is not a physical register if TRI is null |
|
Why do you need to pass in TRI at all? Can't MachineInstr methods always get at it via MF.getSubtarget().getRegisterInfo()? |
I don't think so. This patch is to make the caller explicitly pass the TRI parameter, but you can still call it by passing just a nullptr. |
RKSimon
requested changes
Apr 7, 2024
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
Outdated
Show resolved
Hide resolved
MF data isn't always available - hence why we have the tryToGetTargetInfo workaround. I'm not sure if there are cases where we can provide a valid TRI pointer but tryToGetTargetInfo would have failed? |
|
I'd argue that all these extra nullptr arguments add a lot of churn for no reason unless the plan is to always pass TRI. Can't we just pass the parameter when needed? That would make this patch a lot smaller. |
Maybe, but we're no closer to fixing the problem of people forgetting to provide a TRI and then kill flags etc. get stale. |
simonzgx
force-pushed
the
make_TRI_param_required
branch
3 times, most recently
from
2901c06
to
fc3666b
Compare
done |
|
Does anyone have any other comments please? |
Is the point of the patch to make it slightly harder to accidentally get the wrong behaviour, because you have to explicitly pass If so then no objections from me, though it doesn't seem like a big improvement. |
|
It also moves the TRI argument earlier in the findRegister* calls - while the isKill/isDead/Overlap flag args tend to be optional the TRI is less likely so (plus people have been lazy...). Once this cleanup is in place we're hoping to get most calls with nullptr TRI correctly updated, as we add test coverage. |
|
@simonzgx Please can you rebase again? |
simonzgx
force-pushed
the
make_TRI_param_required
branch
from
fc3666b
to
76d9fb0
Compare
done. |
simonzgx
force-pushed
the
make_TRI_param_required
branch
from
76d9fb0
to
94f9e84
Compare
simonzgx
force-pushed
the
make_TRI_param_required
branch
from
94f9e84
to
1a49155
Compare
|
Any more comments before I push this? |
arsenm
approved these changes
Apr 24, 2024
Yes, it comes down to how thorough we want/need to be with test coverage |
|
@simonzgx Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Hi, [edit] Ignore my comment, it got already fixed [/edit] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #82659
There are some functions, such as
findRegisterDefOperandIdxandfindRegisterDefOperand, that have too many default parameters. As aresult, we have encountered some issues due to the lack of TRI
parameters, as shown in issue #82411.
Following @RKSimon 's suggestion, this patch refactors 9 functions,
including
{reads, kills, defines, modifies}Register,registerDefIsDead, andfindRegister{UseOperandIdx, UseOperand, DefOperandIdx, DefOperand},adjusting the order of the TRI parameter and making it required. In
addition, all the places that call these functions have also been
updated correctly to ensure no additional impact.
After this, the caller of these functions should explicitly know whether
to pass the
TargetRegisterInfoor just anullptr.