[PATCH] D86905: Flush bitcode incrementally for LTO output

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Please mention in the summary somewhere that this is only enabled for lld right now.



================
Comment at: lld/ELF/LTO.cpp:169
+              openLTOOutputFile(config->outputFile))
+        WriteBitcodeToFile(m, *os, false, nullptr, false, nullptr,
+                           config->emitLLVMFlushThreshold);
----------------
Nit, document constant parameters with /*parameter_name=*/


================
Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:154
+    // To reduce disk access, do not fill in Bytes from the file in non-debug
+    // mode and if the data to back path are aligned. At this case, we do not
+    // check existing data and all data will be overwritten.
----------------
s/path/patch/. But I think the whole comment block would be clearer if written in the affirmative sense, e.g. something like:

```
// When unaligned, copy existing data into Bytes from the file FS and the buffer Out so
// that it can be updated before writing. For debug builds read bytes unconditionally 
// in order to check that the existing value is 0 as expected.
```

Also as noted below suggest moving comment just above the #ifdef check below.


================
Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:167
+      ssize_t BytesRead = FS->read(Bytes, BytesFromDisk);
+      (void)BytesRead;
+      assert(BytesRead >= 0 && static_cast<size_t>(BytesRead) == BytesFromDisk);
----------------
Why this line added? Oh ic, presumably to avoid an unused variable warning in the NDEBUG case. Maybe just add a comment to that effect.


================
Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:96
+  void FlushToFile() {
+    constexpr uint64_t FlushThreshold = 1UL << 29; // 512M
+    if (!FS) {
----------------
stephan.yichao.zhao wrote:
> tejohnson wrote:
> > Would it be valuable to make this configurable? How sensitive is performance to the value chosen here?
> Added a flag to plugin-opt.  Is it the right way to do this?
Oh sorry, I just meant an llvm internal option (cl::opt<int>) in this file. Will let @MaskRay comment on whether they want it as an lld option. 


================
Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:153
+
+#ifdef NDEBUG
+    if (StartBit)
----------------
stephan.yichao.zhao wrote:
> tejohnson wrote:
> > Why is this guarded by NDEBUG? I'm not convinced there is much value in doing this code even when StartBit is 0 in the debug case.
> This relates to the assert at line 165.
> 
> At debug mode, the assert at line 165 checks if the value to backpatch is 0. So the code needs to read data from disk.
> 
> At non-debug mode, the assert at line 165 does not verify existing data.  So the code does not need to read data from disk for this reason.
> But if StartBit is non 0, the code still needs to read the existing data because the backpatched value is not aligned.
> For example, when backpatching with StartBit = 2, the aligned data on disk are,
>     c0 00 00 00 3f
> So we need to read them out, fill in those 0s, then write back. Although the code can always read these bytes from disk, I wanted to save some IO overhead.
> 
> Added comments.
Ah ok. I suggest moving that assert up under the #ifdef too then just for clarity, since they go together logically. And as suggested above, move that whole comment about filling in Bytes to here just above the #ifdef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86905