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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl marked 8 inline comments as done.
rahmanl added a comment.

Thanks a lot for the comments @tmsriram . I managed to get most of them addressed (Most importantly the one about nested maps).



================
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;
----------------
tmsriram wrote:
> Move this to MachineFunction.h as a member?
I thought about placing this in https://llvm.org/doxygen/MachineFunction_8h_source.html#l00110 but this is a property of MachineBasicBlock. In fact, MachineFunction does not need to care about section IDs at all.
Is it the "static" keyword which is concerning?


================
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.
----------------
tmsriram wrote:
> Good check, reduces unnecessary section directives if .S is emitted.  Does the entry block begin a section?  If so, it should be skipped.
Yes it does, but it is excluded from this block by pred_empty() check at line 3044.


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:168
     return;
 
   const TargetInstrInfo *TII = MBB.getParent()->getSubtarget().getInstrInfo();
----------------
tmsriram wrote:
> 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?
I agree. It's possible. But the requirement for exception handling blocks might make it a bit complicated. Since this is before basic block sorting, we would have to resort to BBClusterInfo.PositionInCluster which may not represent the ultimate placement of blocks.


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:222
+    } else if (FuncBBClusterInfo.count(MBB.getNumber()))
+      MBB.setSectionID(FuncBBClusterInfo.lookup(MBB.getNumber()).ClusterID);
+    else {
----------------
tmsriram wrote:
> 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.
isBeginSection() and isEndSection() only look at previous and subsequent basic blocks to see if they have a different section ID. This way, we can use SectionIDs to decided if a basic block begins or ends a section without having to store to boolean.


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

https://reviews.llvm.org/D76954