[PATCH] D98976: [CodeGen] Use ProcResGroup information in SchedBoundary

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

Patch looks good. I left a couple of comments below.

Thanks!



================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:678
+  // For each PIdx, stores the resource group IDs of its subunits
+  SmallVector<uint64_t, 16> ResourceGroupSubUnitMasks;
+
----------------
uint64_t is fine in general because models tend to declare less than 30 processor resources on average. On x86 I have seen peaks of ~45 resources (iirc). I don't expect future models to end up declaring more than 63 resources (even mca makes that strong assumption).

That being said, if possible (and if it is not problematic for your code) it would be much safer to use APInt, so that we are not limited to 64-bits and we can manipulate arbitrary big bitmasks.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:2055-2056
       NumUnits += SchedModel->getProcResource(i)->NumUnits;
+      if (SchedModel->getProcResource(i)->SubUnitsIdxBegin &&
+          !SchedModel->getProcResource(i)->BufferSize) {
+        auto SubUnits = SchedModel->getProcResource(i)->SubUnitsIdxBegin;
----------------
Could you move this check into a helper method?
Something like `IsUnbufferedGroup()`. A similar chek is repeated later on in the code; using a helper would make the code slightly more readable.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:2113
 
+  bool IsUnbufferedGroup =
+      SchedModel->getProcResource(PIdx)->SubUnitsIdxBegin &&
----------------
See my previous comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98976/new/

https://reviews.llvm.org/D98976