[PATCH] D16277: Bitcode: use blob for string storage in the IR: trade a bit of space for faster reading

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Thanks for the comments, the measurements were made after the `decodeChar6` changes.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1732
@@ -1729,1 +1731,3 @@
 
+ErrorOr<Value *> BitcodeReader::recordValue(SmallVectorImpl<uint64_t> &Record, StringRef Name,
+                             unsigned NameIndex, Triple &TT) {
----------------
tejohnson wrote:
> There's a lot of code duplication here with the other recordValue(). Looks like the other recordValue() could get the Name string and invoke this one. Or just keep this one and if Name is empty here, invoke convertToString.
I like you second suggestion! Thanks.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2675
@@ -2641,2 +2674,3 @@
     }
-    case bitc::CST_CODE_STRING:    // STRING: [values]
+    case bitc::CST_CODE_STRING:  {  // STRING: [values]
+      if (!Blob.empty()) {
----------------
tejohnson wrote:
> Looks like these two cases could be collapsed as they were before. Is it clearer with them separated?
The CST_CODE_CSTRING has to add a 0 at the end of the string. This was hidden in `ConstantDataArray::getString` at the price of an extra copy. 
This is why I splitted it to save the SmallString copy in the non-blob case for String.
I just notice now that I still have a Smallstring for the STRING case, I think I can remove this.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1643
@@ -1625,1 +1642,3 @@
         AbbrevToUse = CString7Abbrev;
+      if (OptSpeed) {
+        if (Code == bitc::CST_CODE_STRING)
----------------
tejohnson wrote:
> Move up and make other cases "else if"
The `OptSpeed` flag is hacked a bit everywhere, I'll clean it but I wasn't sure about the general feeling with this patch?


http://reviews.llvm.org/D16277