[PATCH] D12536: Function bitcode index in Value Symbol Table and lazy reading support

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Hi Teresa,

Thanks for this patch, it looks good to me overall. 
Please find some comments inline.

Also does it change the behavior of the LazyReader? 
I don't know it enough to know if the fact that you are populating DeferredFunctionInfo using the module VST means it no longer needs to visit all the blocks. I'd expect `llvm-extract` to be //constant time// now, independently of the number of functions in the module.

Best,

Mehdi


================
Comment at: include/llvm/Bitcode/BitstreamReader.h:237
@@ -236,1 +236,3 @@
 
+  unsigned getCurCodeSize() const { return CurCodeSize; }
+
----------------
This is identical to `getAbbrevIDWidth()` two lines above, do you really need to alias it?

================
Comment at: include/llvm/Bitcode/BitstreamWriter.h:126
@@ +125,3 @@
+      CurWord |= NewWord >> (32 - StartBit);
+      support::endian::write32le(&Out[ByteNo], CurWord);
+    }
----------------
(Note: this shift/insert seems ok to me but I am not certain about if it is endian independent)

================
Comment at: include/llvm/Bitcode/BitstreamWriter.h:129
@@ -106,1 +128,3 @@
+  }
+
   void Emit(uint32_t Val, unsigned NumBits) {
----------------
It looks like the `BackpatchWord` change is independent, can it be in a separate commit?

================
Comment at: include/llvm/Bitcode/BitstreamWriter.h:521
@@ -496,1 +520,3 @@
+public:
+
   BlockInfo &getOrCreateBlockInfo(unsigned BlockID) {
----------------
It seems this is not needed for this patch? I don't see any use of `getOrCreateBlockInfo` in the diff?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1589
@@ -1585,2 +1588,3 @@
 
-std::error_code BitcodeReader::parseValueSymbolTable() {
+ErrorOr<Value *> BitcodeReader::recordValue(SmallVector<uint64_t, 64> &Record,
+                                            unsigned NameIndex,
----------------
I believe usually function argument are using the base class `SmallVectorImpl<uint64_t>` to avoid specializing on the size. The size is only relevant when you allocate it.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1693
@@ +1692,3 @@
+        GO = GA->getBaseObject();
+      assert(GO);
+
----------------
Minor comment: You can avoid the extra dyn_cast.

```
      GlobalObject *GO;
      // If this is an alias, need to get the actual Function object
      // it aliases, in order to set up the DeferredFunctionInfo entry below.
      auto *GA = dyn_cast<GlobalAlias>(V);
      if (GA)
        GO = GA->getBaseObject();
      else
        GO = dyn_cast<GlobalObject>(V);
      assert(GO);
```

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2987
@@ +2986,3 @@
+            // of the last function block recorded in the VST (set when
+            // parsing the VST function entries). Skip it.
+            if (Stream.SkipBlock())
----------------
kudos for the comments everywhere! :)

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3301
@@ +3300,3 @@
+    case bitc::MODULE_CODE_VSTOFFSET:
+      if (Record.size() < 1)
+        return error("Invalid record");
----------------
Is `Record.size() > 1` a valid possibility here?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4744
@@ -4625,1 +4743,3 @@
+    // Only should happen if we have old format bitcode.
+    assert(VSTOffset == 0);
     if (Stream.AtEndOfStream())
----------------
Not sure from the comment: is there a possibility for the assert to fire on old format bitcode? LLVM is supposed to always be able to read //old// bitcode.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:599
@@ +598,3 @@
+  Vals.push_back(0);
+  Stream.EmitRecord(bitc::MODULE_CODE_VSTOFFSET, Vals, VSTOffsetAbbrev);
+
----------------
Could be directly:
```
  SmallVector<unsigned, 2> Vals{llvm::bitc::MODULE_CODE_VSTOFFSET, 0};
  Stream.EmitRecordWithAbbrev(VSTOffsetAbbrev, Rec);
```

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2132
@@ -2072,1 +2131,3 @@
+    DenseMap<const Function *, uint64_t> *FunctionIndex = nullptr) {
   if (VST.empty()) return;
+
----------------
What if there has been a placeholder emitted in this case?

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2188
@@ -2080,1 +2187,3 @@
+
+    const ValueName &Name = *SI;
 
----------------

Why did you need to change it to use an iterator?
I believe you can still use `Name` instead of `SI` to have `getValue()` below. 


```
for (const ValueName &Name : VST) {
   Value &Val = Name.getValue();
```


(I may have missed a use-case).

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2203
@@ -2095,1 +2202,3 @@
+    if (GA && GA->getBaseObject())
+      F = dyn_cast<Function>(GA->getBaseObject());
 
----------------
(Minor comment: Same extra `dyn_cast` as previously)


http://reviews.llvm.org/D12536