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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish marked an inline comment as done.
snehasish added a comment.

@efriedma Thanks for the pointers on LLVM string types. Updated the diff with changes, please take a look. Thanks!



================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:91
+      } else {
+        Suffix += "." + std::to_string(SectionID.Number);
+      }
----------------
efriedma wrote:
> 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.
In this case SmallString operator+= accepts a StringRef so just `Suffix += "." + Twine(SectionID.Number);` does not compile. Rewriting this in a different way makes it less readable so I decided to leave it as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79221