[PATCH] D86905: Flush bitcode incrementally for LTO output

[stephan.yichao.zhao via Phabricator via llvm-commits]

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:96
+  void FlushToFile() {
+    constexpr uint64_t FlushThreshold = 1UL << 29; // 512M
+    if (!FS) {
----------------
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?


================
Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:153
+
+#ifdef NDEBUG
+    if (StartBit)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86905