[PATCH] D86905: Flush bitcode incrementally for LTO output

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

There are a number of single statement if and for bodies in the patch that have braces but should not per llvm coding style.



================
Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:153
+
+#ifdef NDEBUG
+    if (StartBit)
----------------
tejohnson wrote:
> 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.
I just realized the first part of this suggestion doesn't make sense, what I should have said is to move the assert up into within the braces, since it goes with that code. I.e. if the code within the braces isn't executed Bytes isn't filled in so there is no point asserting whether Bytes is 0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86905