[PATCH] D79221: Descriptive symbol names for machine basic block sections

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Some minor suggestions for the string manipulation; otherwise looks fine.



================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:83
       CachedMCSymbol =
-          Ctx.getOrCreateSymbol(Prefix + Twine(Delimiter) + "BB" +
-                                Twine(Delimiter) + Twine(MF->getName()));
+          Ctx.getOrCreateSymbol(Prefix + Twine(".BB.") + Twine(MF->getName()));
+    } else if (MF->hasBBSections() && isBeginSection()) {
----------------
We usually prefer something like `Twine(Prefix) + ".BB." + MF->getName()`.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:91
+      } else {
+        Suffix += "." + std::to_string(SectionID.Number);
+      }
----------------
Not that it's likely to matter much, but `Twine(SectionID.Number)` is probably a little more efficient.  (With std::string, the compiler has to work very hard to prove it doesn't need to allocate memory.)

`Ctx.getOrCreateSymbol(MF->getName() + "." + Twine(SectionID.Number))` would also avoid the temporary string, but maybe that's not worth the readability cost.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:97
+                                             Twine(MF->getFunctionNumber()) +
+                                             Twine("_") + Twine(getNumber()));
     }
----------------
You don't need to explicitly cast `"_"` to Twine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79221