Skip to content

Commit 9416f5b

Browse files
dhruvachakaadeshps-mcw
authored andcommitted
[AMDGPU] Fixed crash in getLastMIForRegion when the region is empty. (llvm#168653)
PreRARematStage builds region live-outs if GCN trackers are enabled. If rematerialization leads to empty regions, this can cause a crash because of dereference of an invalid iterator in getLastMIForRegion. The fix is to skip calling getLastMIForRegion for empty regions. This patch fixes another bug in the same code region. getLastMIForRegion calls skipDebugInstructionsBackward which may immediately return the RegionEnd if it is not the begin instruction and it is a non-debug instruction. That would imply considering an instruction that is outside the relevant region. The fix is to always pass the previous of RegionEnd to skipDebugInstructionsBackward. This bug was found while using GCN trackers on the existing LIT test machine-scheduler-sink-trivial-remats.mir. Here's the assertion failure. llvm-project/llvm/include/llvm/ADT/ilist_iterator.h:168: llvm::ilist_iterator<OptionsT, IsReverse, IsConst>::reference llvm::ilist_iterator<OptionsT, IsReverse, IsConst>::operator*() const [with OptionsT = llvm::ilist_detail::node_options<llvm::MachineInstr, true, true, void, false, void>; bool IsReverse = false; bool IsConst = false; llvm::ilist_iterator<OptionsT, IsReverse, IsConst>::reference = llvm::MachineInstr&]: Assertion `!NodePtr->isKnownSentinel()' failed.
1 parent 8d6a016 commit 9416f5b

File tree

2 files changed

+4337
-9
lines changed

2 files changed

+4337
-9
lines changed

llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -978,10 +978,8 @@ GCNScheduleDAGMILive::getRealRegPressure(unsigned RegionIdx) const {
978978

979979
static MachineInstr *getLastMIForRegion(MachineBasicBlock::iterator RegionBegin,
980980
MachineBasicBlock::iterator RegionEnd) {
981-
auto REnd = RegionEnd == RegionBegin->getParent()->end()
982-
? std::prev(RegionEnd)
983-
: RegionEnd;
984-
return &*skipDebugInstructionsBackward(REnd, RegionBegin);
981+
assert(RegionBegin != RegionEnd && "Region must not be empty");
982+
return &*skipDebugInstructionsBackward(std::prev(RegionEnd), RegionBegin);
985983
}
986984

987985
void GCNScheduleDAGMILive::computeBlockPressure(unsigned RegionIdx,
@@ -1076,9 +1074,12 @@ GCNScheduleDAGMILive::getRegionLiveOutMap() const {
10761074
assert(!Regions.empty());
10771075
std::vector<MachineInstr *> RegionLastMIs;
10781076
RegionLastMIs.reserve(Regions.size());
1079-
for (auto &[RegionBegin, RegionEnd] : reverse(Regions))
1077+
for (auto &[RegionBegin, RegionEnd] : reverse(Regions)) {
1078+
// Skip empty regions.
1079+
if (RegionBegin == RegionEnd)
1080+
continue;
10801081
RegionLastMIs.push_back(getLastMIForRegion(RegionBegin, RegionEnd));
1081-
1082+
}
10821083
return getLiveRegMap(RegionLastMIs, /*After=*/true, *LIS);
10831084
}
10841085

@@ -1088,10 +1089,12 @@ void RegionPressureMap::buildLiveRegMap() {
10881089
RegionLiveRegMap =
10891090
IsLiveOut ? DAG->getRegionLiveOutMap() : DAG->getRegionLiveInMap();
10901091
for (unsigned I = 0; I < DAG->Regions.size(); I++) {
1092+
auto &[RegionBegin, RegionEnd] = DAG->Regions[I];
1093+
// Skip empty regions.
1094+
if (RegionBegin == RegionEnd)
1095+
continue;
10911096
MachineInstr *RegionKey =
1092-
IsLiveOut
1093-
? getLastMIForRegion(DAG->Regions[I].first, DAG->Regions[I].second)
1094-
: &*DAG->Regions[I].first;
1097+
IsLiveOut ? getLastMIForRegion(RegionBegin, RegionEnd) : &*RegionBegin;
10951098
IdxToInstruction[I] = RegionKey;
10961099
}
10971100
}

0 commit comments

Comments
 (0)