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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg marked 3 inline comments as done.
thegameg added inline comments.


================
Comment at: llvm/lib/Remarks/BitstreamRemarkSerializer.cpp:342
+
+  Helper.emitRemarkBlock(Remark, *StrTab);
+
----------------
paquette wrote:
> 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.
> 
> 
Correct.

I wouldn't want to move `DidSetUp` into the Helper because I intend to use the Helper in other places, like the generation of the remark section where `emitRemarkBlock` won't be called.

Maybe I should just `assert(DidSetUp == true)` to avoid code getting in between the initialization and the emission of the remark?


================
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/>
----------------
paquette wrote:
> Are these outputs guaranteed to be stable? Judging by the opt runline, it looks like this might rely on inlining behaviour + remark formatting?
You're right, they rely on inlining behavior and on the emission of the remarks from the passes. Ideally we would have unit tests for this, but I'm afraid at that point we'll have to check for byte-by-byte correctness, while here we can sort of have a textual check.

Maybe we should move the whole analysis behavior from `llvm-bcanalyzer` to lib/Bitcode? That way we can have unit tests that come from actual `Remark` objects and can at least compare strings instead of raw bytes.

Let me know if you have any ideas for testing this differently.


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

https://reviews.llvm.org/D63466