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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

Took one pass over the code.

- This needs more tests like with exception blocks kept in separate clusters (check if they are grouped)



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:82
+  // Special section IDs for the cold and exception section
+  const static unsigned ColdSectionID = UINT_MAX;
+  const static unsigned ExceptionSectionID = UINT_MAX - 1;
----------------
Move this to MachineFunction.h as a member?


================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:83
+  const static unsigned ColdSectionID = UINT_MAX;
+  const static unsigned ExceptionSectionID = UINT_MAX - 1;
+
----------------
Also this?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1199
+    if (MBB.isEndSection()) {
+      if (MBB.getSectionID() != MF->front().getSectionID()) {
+        // Emit size directive for the size of this basic block section. The
----------------
Combine the two conditions.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3070
     }
-    // With -fbasicblock-sections, a basic block can start a new section.
-    if (MBB.getSectionType() == MachineBasicBlockSection::MBBS_Exception) {
-      // Create the exception section for this function.
-      OutStreamer->SwitchSection(
-          getObjFileLowering().getNamedSectionForMachineBasicBlock(
-              MF->getFunction(), MBB, TM, ".eh"));
-    } else if (MBB.getSectionType() == MachineBasicBlockSection::MBBS_Cold) {
-      // Create the cold section here.
-      OutStreamer->SwitchSection(
-          getObjFileLowering().getNamedSectionForMachineBasicBlock(
-              MF->getFunction(), MBB, TM, ".unlikely"));
-    } else if (MBB.isBeginSection() && MBB.isEndSection()) {
-      OutStreamer->SwitchSection(
-          getObjFileLowering().getSectionForMachineBasicBlock(MF->getFunction(),
-                                                              MBB, TM));
-    } else if (BBSections) {
-      OutStreamer->SwitchSection(MF->getSection());
+    if (MBB.isBeginSection()) {
+      // With -fbasicblock-sections, a basic block can start a new section.
----------------
Good check, reduces unnecessary section directives if .S is emitted.  Does the entry block begin a section?  If so, it should be skipped.


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:12
 // The purpose of this pass is to assign sections to basic blocks when
-// -fbasicblock-sections= option is used.  Exception landing pad blocks are
-// specially handled by grouping them in a single section.  Further, with
+// -fbasicblock-sections= option is used.  Further, with
 // profile information only the subset of basic blocks with profiles are placed
----------------
Could make longer lines and keep it  better formatted.


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:25
+// To get the optimized performance, the clusters must form an optimal BB layout
+// for the function.
+// Every cluster's section is labeled with a symbol to allow the linker to
----------------
Longer lines?


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:140
+  // name. It returns true if mapping is found and false otherwise.
+  bool getBBClusterInfoForFunction(StringRef s,
+                                   DenseMap<unsigned, BBClusterInfo> &m) {
----------------
Could be made a static function passing the Maps as const reference? 


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:168
     return;
 
   const TargetInstrInfo *TII = MBB.getParent()->getSubtarget().getInstrInfo();
----------------
I see why you removed the sameSection check.  However, when you call this function, you already know the clusterID and the position in the cluster.  So, don't insert the fallthrough if they are the same cluster and are adjacent?


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:213
+    // With the 'list' option, every basic block is placed in a section
+    // associated with its cluster, unless we want sections for every basic
+    // block in this function (if FuncBBClusterInfo is empty).
----------------
"Unless we want individual unique sections for every basic block...."


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:222
+    } else if (FuncBBClusterInfo.count(MBB.getNumber()))
+      MBB.setSectionID(FuncBBClusterInfo.lookup(MBB.getNumber()).ClusterID);
+    else {
----------------
If clusterID is made a pair of integers which denote the ids of the start and end basic blocks, the begin and end function in MachineBasicBlock can be simplified.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:577
 // Returns true if this block begins any section.
 bool MachineBasicBlock::isBeginSection() const {
+  const MachineFunction *MF = getParent();
----------------
This comment applies to isEndSection too.  Consider making beginSection and endSection bool members of MachineBasicBlock and set it once in BBSectionsPrepare.  Isn't this function  called multiple times per MBB,  like for DebugInfo and CFI?  

One way is to make ClusterID a pair of basic block ids that give you the start and end of the cluster.  The beginSection is just "return id == clusterID.first"  and end Section is "return id == clusterID.second"


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

https://reviews.llvm.org/D76954