[llvm-commits] [PATCH] Avoid overflow in scheduling
Reid Kleckner
rnk at mit.edu
Mon Sep 28 17:29:01 PDT 2009
Here's an updated patch. I also switched the numbers to be unsigned
since they should never be negative.
Reid
On Mon, Sep 28, 2009 at 4:26 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>
> On Sep 28, 2009, at 9:45 AM, Dan Gohman wrote:
>
>>
>> On Sep 27, 2009, at 9:17 PM, Reid Kleckner wrote:
>>>
>>> I think the characteristic of this input is that we have 35K BB's in
>>> the function, and each line has a guard that creates a basic block
>>> edge to a single bailing block. When it's time to do scheduling, that
>>> manifests as more than SHRT_MAX predecessors, causing the overflow in
>>> NumSuccs.
>>
>> The NumPreds and NumSuccs values are talking about scheduling
>> units within a basic block, not basic blocks themselves. So it sounds
>> like you not only have large numbers of BBs, but you also have
>> some large individual BBs :-).
>>
>>>
>>> Talking with jyasskin and nlewycky, we think that if LLVM wants the
>>> language restriction that there be no more than SHRT_MAX incoming
>>> edges to a basic block, that's fine, but the verifier should catch it,
>>> because we run the verifier on our code. Otherwise, scheduling
>>> probably needs to support more predecessors.
>>
>> I don't think we want a language restriction like this. I favor changing
>> the
>> fields to ints for now.
>>
>> I think there are some opportunities to make the scheduler use less
>> memory that we can pursue instead of trying to get by with short fields.
>
> Alright.
>
> Evan
>
>>
>> Dan
>>
>
>
-------------- next part --------------
Index: include/llvm/CodeGen/ScheduleDAG.h
===================================================================
--- include/llvm/CodeGen/ScheduleDAG.h (revision 83034)
+++ include/llvm/CodeGen/ScheduleDAG.h (working copy)
@@ -243,10 +243,10 @@
unsigned NodeNum; // Entry # of node in the node vector.
unsigned NodeQueueId; // Queue id of node.
unsigned short Latency; // Node latency.
- short NumPreds; // # of SDep::Data preds.
- short NumSuccs; // # of SDep::Data sucss.
- short NumPredsLeft; // # of preds not scheduled.
- short NumSuccsLeft; // # of succs not scheduled.
+ unsigned NumPreds; // # of SDep::Data preds.
+ unsigned NumSuccs; // # of SDep::Data sucss.
+ unsigned NumPredsLeft; // # of preds not scheduled.
+ unsigned NumSuccsLeft; // # of succs not scheduled.
bool isTwoAddress : 1; // Is a two-address instruction.
bool isCommutable : 1; // Is a commutable instruction.
bool hasPhysRegDefs : 1; // Has physreg defs that are being used.
Index: lib/CodeGen/ScheduleDAG.cpp
===================================================================
--- lib/CodeGen/ScheduleDAG.cpp (revision 83034)
+++ lib/CodeGen/ScheduleDAG.cpp (working copy)
@@ -82,13 +82,19 @@
SUnit *N = D.getSUnit();
// Update the bookkeeping.
if (D.getKind() == SDep::Data) {
+ assert(NumPreds < UINT_MAX && "NumPreds will overflow!");
+ assert(N->NumSuccs < UINT_MAX && "NumSuccs will overflow!");
++NumPreds;
++N->NumSuccs;
}
- if (!N->isScheduled)
+ if (!N->isScheduled) {
+ assert(NumPredsLeft < UINT_MAX && "NumPredsLeft will overflow!");
++NumPredsLeft;
- if (!isScheduled)
+ }
+ if (!isScheduled) {
+ assert(N->NumSuccsLeft < UINT_MAX && "NumSuccsLeft will overflow!");
++N->NumSuccsLeft;
+ }
Preds.push_back(D);
N->Succs.push_back(P);
if (P.getLatency() != 0) {
@@ -121,13 +127,19 @@
Preds.erase(I);
// Update the bookkeeping.
if (P.getKind() == SDep::Data) {
+ assert(NumPreds > 0 && "NumPreds will underflow!");
+ assert(N->NumSuccs > 0 && "NumSuccs will underflow!");
--NumPreds;
--N->NumSuccs;
}
- if (!N->isScheduled)
+ if (!N->isScheduled) {
+ assert(NumPredsLeft > 0 && "NumPredsLeft will underflow!");
--NumPredsLeft;
- if (!isScheduled)
+ }
+ if (!isScheduled) {
+ assert(N->NumSuccsLeft > 0 && "NumSuccsLeft will underflow!");
--N->NumSuccsLeft;
+ }
if (P.getLatency() != 0) {
this->setDepthDirty();
N->setHeightDirty();
Index: lib/CodeGen/PostRASchedulerList.cpp
===================================================================
--- lib/CodeGen/PostRASchedulerList.cpp (revision 83034)
+++ lib/CodeGen/PostRASchedulerList.cpp (working copy)
@@ -966,17 +966,17 @@
/// the PendingQueue if the count reaches zero. Also update its cycle bound.
void SchedulePostRATDList::ReleaseSucc(SUnit *SU, SDep *SuccEdge) {
SUnit *SuccSU = SuccEdge->getSUnit();
- --SuccSU->NumPredsLeft;
-
+
#ifndef NDEBUG
- if (SuccSU->NumPredsLeft < 0) {
+ if (SuccSU->NumPredsLeft == 0) {
errs() << "*** Scheduling failed! ***\n";
SuccSU->dump(this);
errs() << " has been released too many times!\n";
llvm_unreachable(0);
}
#endif
-
+ --SuccSU->NumPredsLeft;
+
// Compute how many cycles it will be before this actually becomes
// available. This is the max of the start time of all predecessors plus
// their latencies.
Index: lib/CodeGen/SelectionDAG/ScheduleDAGList.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/ScheduleDAGList.cpp (revision 83034)
+++ lib/CodeGen/SelectionDAG/ScheduleDAGList.cpp (working copy)
@@ -108,17 +108,17 @@
/// the PendingQueue if the count reaches zero. Also update its cycle bound.
void ScheduleDAGList::ReleaseSucc(SUnit *SU, const SDep &D) {
SUnit *SuccSU = D.getSUnit();
- --SuccSU->NumPredsLeft;
-
+
#ifndef NDEBUG
- if (SuccSU->NumPredsLeft < 0) {
+ if (SuccSU->NumPredsLeft == 0) {
errs() << "*** Scheduling failed! ***\n";
SuccSU->dump(this);
errs() << " has been released too many times!\n";
llvm_unreachable(0);
}
#endif
-
+ --SuccSU->NumPredsLeft;
+
SuccSU->setDepthToAtLeast(SU->getDepth() + D.getLatency());
// If all the node's predecessors are scheduled, this node is ready
Index: lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp (revision 83034)
+++ lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp (working copy)
@@ -278,6 +278,7 @@
AvailableQueue->remove(PredSU);
}
+ assert(PredSU->NumSuccsLeft < UINT_MAX && "NumSuccsLeft will overflow!");
++PredSU->NumSuccsLeft;
}
@@ -824,17 +825,17 @@
/// the AvailableQueue if the count reaches zero. Also update its cycle bound.
void ScheduleDAGRRList::ReleaseSucc(SUnit *SU, const SDep *SuccEdge) {
SUnit *SuccSU = SuccEdge->getSUnit();
- --SuccSU->NumPredsLeft;
-
+
#ifndef NDEBUG
- if (SuccSU->NumPredsLeft < 0) {
+ if (SuccSU->NumPredsLeft == 0) {
errs() << "*** Scheduling failed! ***\n";
SuccSU->dump(this);
errs() << " has been released too many times!\n";
llvm_unreachable(0);
}
#endif
-
+ --SuccSU->NumPredsLeft;
+
// If all the node's predecessors are scheduled, this node is ready
// to be scheduled. Ignore the special ExitSU node.
if (SuccSU->NumPredsLeft == 0 && SuccSU != &ExitSU) {
Index: lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp (revision 83034)
+++ lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp (working copy)
@@ -134,17 +134,17 @@
/// the AvailableQueue if the count reaches zero. Also update its cycle bound.
void ScheduleDAGFast::ReleasePred(SUnit *SU, SDep *PredEdge) {
SUnit *PredSU = PredEdge->getSUnit();
- --PredSU->NumSuccsLeft;
-
+
#ifndef NDEBUG
- if (PredSU->NumSuccsLeft < 0) {
+ if (PredSU->NumSuccsLeft == 0) {
errs() << "*** Scheduling failed! ***\n";
PredSU->dump(this);
errs() << " has been released too many times!\n";
llvm_unreachable(0);
}
#endif
-
+ --PredSU->NumSuccsLeft;
+
// If all the node's successors are scheduled, this node is ready
// to be scheduled. Ignore the special EntrySU node.
if (PredSU->NumSuccsLeft == 0 && PredSU != &EntrySU) {
More information about the llvm-commits
mailing list