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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 7 18:32:40 PDT 2020


efriedma added a comment.

I'm happy with this at a high level; it's not really any more complicated than BasicBlock sections itself, and the added flexibility makes sense.

>From the AsmPrinter's perspective, there isn't any difference between having BBSections disabled, vs. having every BB in the function assigned to the section with the same ID.  You shouldn't really need to explicitly check whether 
BBSections are enabled from the AsmPrinter; all the checks that do remain are suspicious.

Also along those lines, does the SectionID need to be `Optional<unsigned>`, as opposed to just being `unsigned`?



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:490
+  bool sameSection(const MachineBasicBlock *MBB) const {
+    return this == MBB || getSectionID() == MBB->getSectionID();
+  }
----------------
"this == MBB" check is redundant, I think.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1181
+
+    if (MF->hasBBLabels() ||
+        (MBB.isEndSection() && !MBB.sameSection(&MF->front()))) {
----------------
I don't like the way this if statement is written.  The fundamental MCBinaryExpr::createSub() operation is shared, but the two operations aren't really related anymore, so the logic here is sort of twisted.  I'd suggest pulling the createTempSymbol()/MCBinaryExpr::createSub()/emitELFSize() into a helper (maybe a lambda), and calling it from two separate if statements.


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:630
+    if (getUnsigned(Value))
+      return error("Uknown Section ID");
+    SID = Value;
----------------
"unknown"


================
Comment at: llvm/lib/CodeGen/MIRParser/MIRParser.cpp:51
+/// and the SectionID's are assigned to MBBs.
+static void assignBeginEndSections(MachineFunction &MF) {
+  MF.front().setIsBeginSection();
----------------
Why are there two copies of assignBeginEndSections ?


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

https://reviews.llvm.org/D76954





More information about the llvm-commits mailing list