[PATCH] D31838: Bitcode: Add a string table to the bitcode format.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5013
       break;
+    case bitc::FS_VALUE_GUID: {
+      uint64_t ValueID = Record[0];
----------------
Add comment with record format above here (see other cases).


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1285
 
-  // Emit the module's source file name.
-  {
----------------
Why does this move?


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1310
-
-  // If we have a VST, write the VSTOFFSET record placeholder.
-  if (M.getValueSymbolTable().empty())
----------------
Why does this go away?


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2902
 
-    // 6-bit char6 VST_CODE_FNENTRY function strings.
-    Abbv = std::make_shared<BitCodeAbbrev>();
-    Abbv->Add(BitCodeAbbrevOp(bitc::VST_CODE_FNENTRY));
-    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8)); // value id
-    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8)); // funcoffset
-    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
-    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Char6));
-    FnEntry6BitAbbrev = Stream.EmitAbbrev(std::move(Abbv));
+/// Emit names for globals/functions etc. \p IsModuleLevel is true when
+/// we are writing the module-level VST, where we are including a function
----------------
Comment needs updating.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2905
+/// bitcode index and need to backpatch the VST forward declaration record.
+void ModuleBitcodeWriter::writeValueSymbolTable(const ValueSymbolTable &VST) {
+  if (VST.empty())
----------------
Maybe rename to writeFunctionLevelValueSymbolTable or something like that to distinguish from the other one.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2924
     // VST_CODE_ENTRY:   [valueid, namechar x N]
     // VST_CODE_FNENTRY: [valueid, funcoffset, namechar x N]
     // VST_CODE_BBENTRY: [bbid, namechar x N]
----------------
This one goes away I think


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3539
+      for (auto &RI : VS->refs())
+        getValueId(RI.getGUID());
+      continue;
----------------
Can getValueId be split into two, part that does the assignment if it doesn't exist in map and returns nothing (called from this code), and one that asserts it exists in map and returns the value (called from where we originally called getValueId further down? Presumably by the time we reach there we will always have them assigned due to this code.


================
Comment at: llvm/tools/llvm-cat/llvm-cat.cpp:54
   } else {
+    std::vector<std::unique_ptr<Module>> OwnedMods;
     for (std::string InputFilename : InputFilenames) {
----------------
Needed because string in string table owned by parsed modules?


================
Comment at: llvm/tools/llvm-modextract/llvm-modextract.cpp:64
     BitcodeWriter Writer(Header);
-    Out->os() << Header << Ms[ModuleIndex].getBuffer();
+    Header.append(Ms[ModuleIndex].getBuffer().begin(),
+                  Ms[ModuleIndex].getBuffer().end());
----------------
Rename variable? "Header" no longer seems appropriate.


https://reviews.llvm.org/D31838