[PATCH] D76954: LLVM support for BB-cluster sections

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added inline comments.


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:158
+// This function updates and optimizes the branching instructions of every basic
+// block in a given function to accout for changes in the layout.
+static void updateBranches(
----------------
s/accout/account


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:264
+  // exception section, as we need all EH Pads to be in a single section.
+  if (EHPadsSections.size() > 1) {
     std::for_each(MF.begin(), MF.end(), [&](MachineBasicBlock &MBB) {
----------------
Does this need a set?  could you make EHPadSections a integer = -1.  Set it to the clusterid when you see the first exception MBB.  After that, if you see another MBB with a different clusterid, set it to MachineBasicBlock::ExcpetionSectionID and check for this before resetting?


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:290
+    if (XSectionID == YSectionID)
+      return XSectionID < MachineBasicBlock::ExceptionSectionID
+                 ? FuncBBClusterInfo[X.getNumber()]->PositionInCluster <
----------------
Wouldn't  this a bit more clear:

return (XSectionID == ExceptionSectionID || XSectionID == ColdSectionID) ? X.getNumber() < Y.getNumber : ...PositionInCluster < ...PositionInCluster

The implicit ColdSectionID > ExceptionSectionID should be exposed unless they are part of enum I think.


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:304
+  // when possible.
+  updateBranches(MF, PreLayoutFallThroughs);
 
----------------
I get why you are doing it here now.  You want to generate the optimal branches when possible, something you couldn't do before.


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:362
+    ErrorMessage += ": ";
+    ErrorMessage += Message;
+    return make_error<StringError>(ErrorMessage, inconvertibleErrorCode());
----------------
Why not make it one expression?



================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:389
+            "Cluster list does not follow a function name specifier.");
+      std::istringstream ISS(S.str());
+      // Reset current cluster position.
----------------
I dont see any other uses of istringstream except in a tool.  Why not just use .split(" ")?


================
Comment at: llvm/lib/CodeGen/MIRPrinter.cpp:648
+    case MachineBasicBlock::ExceptionSectionID:
+      OS << "Excetion";
       break;
----------------
s/Excetion/Exception


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

https://reviews.llvm.org/D76954