-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[regalloc][LiveRegMatrix][AMDGPU] Fix LiveInterval dangling pointers in LiveRegMatrix. #168556
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -79,6 +79,19 @@ void LiveIntervalUnion::extract(const LiveInterval &VirtReg, | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| void LiveIntervalUnion::clearAllSegmentsReferencing( | ||||||
| const LiveInterval &VirtReg) { | ||||||
| ++Tag; | ||||||
|
|
||||||
| // Remove all segments referencing VirtReg. | ||||||
| for (SegmentIter SegPos = Segments.begin(); SegPos.valid();) { | ||||||
| if (SegPos.value() == &VirtReg) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| SegPos.erase(); | ||||||
| else | ||||||
| ++SegPos; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| void | ||||||
| LiveIntervalUnion::print(raw_ostream &OS, const TargetRegisterInfo *TRI) const { | ||||||
| if (empty()) { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,11 +12,13 @@ | |
|
|
||
| #include "llvm/CodeGen/LiveRegMatrix.h" | ||
| #include "RegisterCoalescer.h" | ||
| #include "llvm/ADT/DenseSet.h" | ||
| #include "llvm/ADT/Statistic.h" | ||
| #include "llvm/CodeGen/LiveInterval.h" | ||
| #include "llvm/CodeGen/LiveIntervalUnion.h" | ||
| #include "llvm/CodeGen/LiveIntervals.h" | ||
| #include "llvm/CodeGen/MachineFunction.h" | ||
| #include "llvm/CodeGen/MachineRegisterInfo.h" | ||
| #include "llvm/CodeGen/TargetRegisterInfo.h" | ||
| #include "llvm/CodeGen/TargetSubtargetInfo.h" | ||
| #include "llvm/CodeGen/VirtRegMap.h" | ||
|
|
@@ -142,6 +144,20 @@ void LiveRegMatrix::unassign(const LiveInterval &VirtReg) { | |
| LLVM_DEBUG(dbgs() << '\n'); | ||
| } | ||
|
|
||
| void LiveRegMatrix::unassign(Register VirtReg) { | ||
| Register PhysReg = VRM->getPhys(VirtReg); | ||
| LLVM_DEBUG(dbgs() << "unassigning " << printReg(VirtReg, TRI) << " from " | ||
| << printReg(PhysReg, TRI) << '\n'); | ||
| VRM->clearVirt(VirtReg); | ||
|
|
||
| assert(LIS->hasInterval(VirtReg)); | ||
| const LiveInterval &LI = LIS->getInterval(VirtReg); | ||
| for (MCRegUnit Unit : TRI->regunits(PhysReg)) { | ||
| Matrix[Unit].clearAllSegmentsReferencing(LI); | ||
| } | ||
|
Comment on lines
+151
to
+157
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this share code with the LiveInterval form |
||
| ++NumUnassigned; | ||
| } | ||
|
|
||
| bool LiveRegMatrix::isPhysRegUsed(MCRegister PhysReg) const { | ||
| for (MCRegUnit Unit : TRI->regunits(PhysReg)) { | ||
| if (!Matrix[Unit].empty()) | ||
|
|
@@ -290,6 +306,33 @@ Register LiveRegMatrix::getOneVReg(unsigned PhysReg) const { | |
| return MCRegister::NoRegister; | ||
| } | ||
|
|
||
| bool LiveRegMatrix::isValid() const { | ||
| // Build set of all valid LiveInterval pointers from LiveIntervals. | ||
| DenseSet<LiveInterval *> ValidIntervals; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const LiveInterval * ? |
||
| for (unsigned RegIdx = 0, NumRegs = VRM->getRegInfo().getNumVirtRegs(); | ||
| RegIdx < NumRegs; ++RegIdx) { | ||
| Register VReg = Register::index2VirtReg(RegIdx); | ||
| // Only track assigned registers since unassigned ones won't be in Matrix | ||
| if (VRM->hasPhys(VReg) && LIS->hasInterval(VReg)) | ||
| ValidIntervals.insert(&LIS->getInterval(VReg)); | ||
| } | ||
|
|
||
| // Now scan all LiveIntervalUnions in the matrix and verify each pointer | ||
| unsigned NumDanglingPointers = 0; | ||
| for (unsigned I = 0, Size = Matrix.size(); I != Size; ++I) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: inconsistent condition in these two loops, |
||
| MCRegUnit Unit = static_cast<MCRegUnit>(I); | ||
| for (const LiveInterval *LI : Matrix[Unit]) { | ||
| if (!ValidIntervals.contains(LI)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const LiveInterval *LI => typeof(ValidIntervals) == DenseSet<const LiveInterval *> ? |
||
| ++NumDanglingPointers; | ||
| dbgs() << "ERROR: LiveInterval pointer is not found in LiveIntervals:\n" | ||
| << " Register Unit: " << printRegUnit(Unit, TRI) << '\n' | ||
| << " LiveInterval pointer: " << LI << '\n'; | ||
|
Comment on lines
+327
to
+329
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unguarded debug printing, make it an assert? |
||
| } | ||
| } | ||
| } | ||
| return NumDanglingPointers == 0; | ||
| } | ||
|
|
||
| AnalysisKey LiveRegMatrixAnalysis::Key; | ||
|
|
||
| LiveRegMatrix LiveRegMatrixAnalysis::run(MachineFunction &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.
Matrix can be null.
llvm/lib/CodeGen/RegAllocPBQP.cpp:811
std::unique_ptr<Spiller> VRegSpiller( createInlineSpiller({LIS, LiveStks, MDT, MBFI}, MF, VRM, DefaultVRAI));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.
Kind of a problem, but we don't handle PBQP today (e.g., it doesn't support the regalloc splitting)