[PATCH] D63466: [Remarks] Add an LLVM-bitstream-based remark serializer

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/Remarks/BitstreamRemarkContainer.h:32-34
+/// * separate: the metadata is separate from the remarks and points to the
+///             auxiliary file that contains the remarks.
+/// * standalone: the metadata and the remarks are emitted together.
----------------
A couple things:

1) Is remark metadata explicitly documented anywhere? The comment in BitstreamRemarkSerializer.h seems to be about half way there. Maybe it would be good to expand a bit upon that, and then reference that file here? (I think it might be worth adding to the LLVM docs on remarks eventually.)

2) The "separate" point only talks about `SeparateRemarksFile`?





================
Comment at: llvm/include/llvm/Remarks/BitstreamRemarkContainer.h:52
+  /// * String table
+  Standalone,
+};
----------------
extra comma


================
Comment at: llvm/include/llvm/Remarks/BitstreamRemarkContainer.h:55
+
+enum BlockIDs {
+  BLOCK_META = bitc::FIRST_APPLICATION_BLOCKID,
----------------
Can we have a documentation comment on this?


================
Comment at: llvm/include/llvm/Remarks/BitstreamRemarkContainer.h:63
+
+enum RecordIDs {
+  // Meta block records.
----------------
A documentation comment would be useful here as well.


================
Comment at: llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h:62-63
+  /// Note: depending on the container type, some IDs might be uninitialized.
+  /// Warning: When adding more abbrev IDs, make sure to update the
+  /// BlockCodeSize (in the call to EnterSubblock).
+  uint64_t RecordMetaContainerInfoAbbrevID = 0;
----------------
Is it possible to verify this with a test?

E.g. add a debug dumper for the available abbrev IDs and do something similar to llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir?


================
Comment at: llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h:76
+
+  /// Setup the necessary block info entries according to the container type.
+  void setupBlockInfo();
----------------
Grammar nit: //Setup// is a noun. I would use //set up// here. English is weird.


================
Comment at: llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h:81
+  void setupMetaBlockInfo();
+  /// The remark version in the metadata block.
+  void setupMetaRemarkVersion();
----------------
This, and the other ones should probably say "set up" like `setupMetaBlockInfo`


================
Comment at: llvm/lib/Remarks/BitstreamRemarkSerializer.cpp:23-26
+static void push(SmallVectorImpl<uint64_t> &R, StringRef Str) {
+  for (const char C : Str)
+    R.push_back(C);
+}
----------------
Whenever this function is called, there's a call to `Bitstream.EmitRecord` following it. Would it make sense to pull that into the function and give it a more descriptive name?


================
Comment at: llvm/lib/Remarks/BitstreamRemarkSerializer.cpp:30-32
+  R.clear();
+  R.push_back(BLOCK_META);
+  Bitstream.EmitRecord(bitc::BLOCKINFO_CODE_SETBID, R);
----------------
This is a pretty common pattern as well. Could this be pulled out into a function?


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

https://reviews.llvm.org/D63466