[PATCH] D86644: [llvm-mca] Initial implementation of output serialization using JSON

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

I can see that `InstructionView` has been moved outside of View.h into its own file, but I cannot see that new file anywhere in this diff.

I think that this diff is missing View/InstructionView.{h,cpp}, and llvm/Support/JSON.h.

Could you please upload your full patch?

Some minor comments below.



================
Comment at: llvm/tools/llvm-mca/PipelinePrinter.cpp:16
 #include "Views/View.h"
+#include "llvm/Support/JSON.h"
 
----------------
Is it necessary to include this file here?
Also, I cannot see that file in the diff. Could you please upload a full diff (assuming that this is not a phab user error... :) ).

My point is that there is nothing specific about this file which is dependent on any definitions other than those from View.h.

Also, some views might end up implicitly including it anyway.
I noticed that most views tend to include InstructionView.h (which unfortunately I am unable to see in this diff). Maybe you could move this include in there?



================
Comment at: llvm/tools/llvm-mca/Views/ResourcePressureView.h:74
 class ResourcePressureView : public InstructionView {
+protected:
   unsigned LastInstructionIdx;
----------------
Is protected really necessary here? It seems to me that it should be private.


================
Comment at: llvm/tools/llvm-mca/Views/TimelineView.h:127
 
+protected:
   struct TimelineViewEntry {
----------------
Why protected?
Please correct me if I am wrong, but I don't think it needs to be protected and it can stay private.


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

https://reviews.llvm.org/D86644