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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Remarks/BitstreamRemarkSerializer.cpp:342
+
+  Helper.emitRemarkBlock(Remark, *StrTab);
+
----------------
This should only run when `DidSetUp == true`, correct?

Would it make sense to move `DidSetUp` into the Helper? Then you could set it in `setupBlockInfo` and either assert or throw an error if `DidSetUp == false` in appropriate Helper functions.




================
Comment at: llvm/test/CodeGen/X86/remarks-bitstream.ll:11
+; LLC-NEXT: </Meta>
+; LLC-NEXT: <Remark NumWords=3 BlockCodeSize=4>
+; LLC-NEXT:   <Remark header abbrevid=4 op0=3 op1=0 op2=1 op3=2/>
----------------
Are these outputs guaranteed to be stable? Judging by the opt runline, it looks like this might rely on inlining behaviour + remark formatting?


================
Comment at: llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp:975
+  case LLVMBitstreamRemarks:
+    outs() << "LLVM Bitstream Remarks\n";
+    break;
----------------
"Bitstream" seems a little redundant here. Just "LLVM Remarks" instead?


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

https://reviews.llvm.org/D63466