[PATCH] D70650: [MC] Rewrite tablegen for printInstrAlias to comiple faster, NFC

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 3 inline comments as done.
rnk added inline comments.


================
Comment at: llvm/lib/MC/MCInstPrinter.cpp:106
+                                              const AliasMatchingData &M) {
+  // FIXME: To expensive to do every call. Check ctor?
+  assert(std::is_sorted(
----------------
craig.topper wrote:
> Nit: I think the first "To" was supposed to be "Too"
Actually, this wasn't suppose to go up for review. I don't want to do `O(n)` things that should be `O(log n)` even in an asserts build. I'll try moving this assert into a static local constructor that runs once.


================
Comment at: llvm/utils/TableGen/AsmWriterEmitter.cpp:682
     StringRef ASM(AsmString);
-    SmallString<128> OutString;
-    raw_svector_ostream OS(OutString);
+    std::string OutString;
+    raw_string_ostream OS(OutString);
----------------
craig.topper wrote:
> Was this changed from SmallString to std::string so we could just return it? Do we lose out on NRVO by going through OS.str() instead of OS.flush; return OutString;?
Yes, I think you're right.


================
Comment at: llvm/utils/TableGen/AsmWriterEmitter.cpp:685
     for (StringRef::iterator I = ASM.begin(), E = ASM.end(); I != E;) {
       OS << *I;
+      ++UnescapedSize;
----------------
craig.topper wrote:
> How efficient is putting 1 character at a time into a std::string with respect to heap allocations vs the large SmallString we had before. Though I'm not sure how long these strings are in practice. Do we stay under the small string size for std::string?
Well, this is tablegen anyway, so efficiency is less of a concern. If we wanted to save tablegen time, there are a number of things we could do to speed it up. These strings do end up being under 128 characters in practice, since they are derived from assembler snippets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70650