From b39173e609f324c4f7490fe3d85282d1c6fca2ab Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Tue, 21 May 2024 17:14:33 -0700 Subject: [PATCH] [MISched][NFC] Add documentation comment in pickNode for ReadyQueue maintenence I had some trouble understanding why `removeReady` removed nodes from the Pending queue, since my intuition told me that the Pending queue did not represent a node that was ready. I took a deeper look and found that pickOnlyNode and pickNodeFromQueue only picked nodes from the Available queue too. I found that need to nodes from the Available and Pending queues that correspond to the opposite direction that we ended up choosing from (IsTopNode vs !IsTopNode). It took me a little longer than I would have liked to understand this fact, so I figured that I would add a comment in the code that makes it clear for future readers. --- llvm/lib/CodeGen/MachineScheduler.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/llvm/lib/CodeGen/MachineScheduler.cpp b/llvm/lib/CodeGen/MachineScheduler.cpp index 0858be64de405b1..03e892a5e0d2259 100644 --- a/llvm/lib/CodeGen/MachineScheduler.cpp +++ b/llvm/lib/CodeGen/MachineScheduler.cpp @@ -3777,6 +3777,21 @@ SUnit *GenericScheduler::pickNode(bool &IsTopNode) { } } while (SU->isScheduled); + // If IsTopNode, then SU is in Top.Available and must be removed. Otherwise, + // if isTopReady(), then SU is in either Top.Available or Top.Pending. + // If !IsTopNode, then SU is in Bot.Available and must be removed. Otherwise, + // if isBottomReady(), then SU is in either Bot.Available or Bot.Pending. + // + // It is coincidental when !IsTopNode && isTopReady or when IsTopNode && + // isBottomReady. That is, it didn't factor into the decision to choose SU + // because it isTopReady or isBottomReady, respectively. In fact, if the + // RegionPolicy is OnlyTopDown or OnlyBottomUp, then the Bot queues and Top + // queues respectivley contain the original roots and don't get updated when + // picking a node. So if SU isTopReady on a OnlyBottomUp pick, then it was + // because we schduled everything but the top roots. Conversley, if SU + // isBottomReady on OnlyTopDown, then it was because we scheduled everything + // but the bottom roots. If its in a queue even coincidentally, it should be + // removed so it does not get re-picked in a subsequent pickNode call. if (SU->isTopReady()) Top.removeReady(SU); if (SU->isBottomReady())