[PATCH] D48164: [llvm-exegesis] Print the whole snippet in analysis.

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet requested changes to this revision.
gchatelet added inline comments.
This revision now requires changes to proceed.


================
Comment at: tools/llvm-exegesis/lib/Analysis.cpp:94
+                            const char *Separator) const {
+  llvm::SmallVector<std::string, 3> Lines;
+  // Parse the asm snippet and print it.
----------------
Why not a `llvm::SmallVector<llvm::SmallString<16>, 3>` ?


================
Comment at: tools/llvm-exegesis/lib/Analysis.cpp:110
+    InstPrinter_->printInst(&MI, OSS, "", *SubtargetInfo_);
+    Bytes = Bytes.slice(MISize);
+    OSS.flush();
----------------
as discussed please use `drop_front()` which is more self explanatory.


================
Comment at: tools/llvm-exegesis/lib/Analysis.h:56
 
     const llvm::MCSchedClassDesc &SCDesc;
     const llvm::SmallVector<llvm::MCWriteProcResEntry, 8>
----------------
[nit] as discussed you may want to use a pointer here to make it clear that llvm::MCSchedClassDesc is immutable.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkResult.cpp:161
+    {
+      BinaryRef Binary;
+      if (Io.outputting()) {
----------------
You may use the YAML Normalization https://llvm.org/docs/YamlIO.html#normalization


Repository:
  rL LLVM

https://reviews.llvm.org/D48164