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

[Sriraman Tallam via Phabricator via llvm-commits]

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

Please do not submit without an approval from @efriedma as they approved the original patch.

This LGTM with these changes.



================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:261
 
-  // If some EH Pads are not cold then we move all EH Pads to the exception
-  // section as we require that all EH Pads be in a single section.
-  if (HasHotEHPads) {
-    std::for_each(MF.begin(), MF.end(), [&](MachineBasicBlock &MBB) {
-      if (MBB.isEHPad())
-        MBB.setSectionType(MachineBasicBlockSection::MBBS_Exception);
-    });
+    if (MBB.isEHPad() && MBB.getSectionID().getValue() != EHPadsSectionID)
+      EHPadsSectionID = EHPadsSectionID == -1
----------------
You can also add EHPadsSectionID != ExceptionSectionID


================
Comment at: llvm/lib/CodeGen/BBSectionsPrepare.cpp:270
   for (auto &MBB : MF) {
-    // With -fbasicblock-sections, fall through blocks must be made
-    // explicitly reachable.  Do this after sections is set as
-    // unnecessary fallthroughs can be avoided.
-    insertUnconditionalFallthroughBranch(MBB);
+    if (EHPadsSectionID == MachineBasicBlock::ExceptionSectionID &&
+        MBB.isEHPad())
----------------
EHPadsSectionID == MachineBasicBlock::ExceptionSectionID should be moved outside the loop.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:552
 // Returns true if this basic block and the Other are in the same section.
 bool MachineBasicBlock::sameSection(const MachineBasicBlock *Other) const {
+  return this->getSectionID() == Other->getSectionID();
----------------
This has become too simple, maybe just delete the function or move the body to the header file itself?


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:374
+/// and the SectionID's are assigned to MBBs.
+void MachineFunction::assignBeginEndSections() {
+  assert(hasBBSections());
----------------
This function is called twice and I highly doubt this will be called more than that. Once in BBSectionsPrepare.cpp and other in MIRParser.  Should it be a member?  You can repeat it in both places I think, it is small. 

Also, you could initialize isBeginSection and isEndSection to false and just handle the true case maybe.


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

https://reviews.llvm.org/D76954