[llvm] 71b1fbd - [MISched][NFC] Add documentation comment in pickNode for ReadyQueue maintenence (#92976)

[via llvm-commits]

Author: Michael Maitland
Date: 2024-05-22T08:27:35-04:00
New Revision: 71b1fbdff6cf567ad278c51f0acdcdf23de0ac28

URL: https://github.com/llvm/llvm-project/commit/71b1fbdff6cf567ad278c51f0acdcdf23de0ac28
DIFF: https://github.com/llvm/llvm-project/commit/71b1fbdff6cf567ad278c51f0acdcdf23de0ac28.diff

LOG: [MISched][NFC] Add documentation comment in pickNode for ReadyQueue maintenence (#92976)

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.

Added: 
    

Modified: 
    llvm/lib/CodeGen/MachineScheduler.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/MachineScheduler.cpp b/llvm/lib/CodeGen/MachineScheduler.cpp
index 0858be64de405..03e892a5e0d22 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())