[PATCH] D105064: [llvm-mca] Fix JSON output

[Marcos Horro via Phabricator via llvm-commits]

markoshorro added a comment.

@andreadb I understand you were trying to go straight to the point, I am a newbie, and I need to learn :-)
When I have time, I will consider updating the other format changes in a new patch.

Thanks.



================
Comment at: llvm/docs/CommandGuide/llvm-mca.rst:214-216
+  The individual views refer to them by index. However, not all views are
+  supported. Unsupported views are not printed, such as bottleneck analysis,
+  etc.
----------------
andreadb wrote:
> "However, not all views are currently supported. For example, the report from the bottleneck analysis is not printed out in json. All the default views are currently supported."
> 
Noted.


================
Comment at: llvm/test/tools/llvm-mca/JSON/X86/views.s:57-79
+# CHECK-NEXT:   "Instructions and CPU resources": {
+# CHECK-NEXT:     "Instructions": [
+# CHECK-NEXT:       "addl\t%eax, %eax",
+# CHECK-NEXT:       "addl\t%ebx, %ebx",
+# CHECK-NEXT:       "addl\t%ecx, %ecx",
+# CHECK-NEXT:       "addl\t%edx, %edx"
+# CHECK-NEXT:     ],
----------------
andreadb wrote:
> Why do we nest the name/value pairs for "Resources" and "Instructions" under "Instructions and CPU resources"?
> Those should be at the top, and ideally they should not be nested under anything else.
> 
> 
This is because they belong to InstructionView and since the printReport() function performs `json::Object.try_emplace(View->getNameAsString().str(), View->toJSON());` for all Views, then it nests those pairs under that name. Should I change it anyways?


================
Comment at: llvm/tools/llvm-mca/Views/View.h:19-22
 #include "llvm/MCA/HWEventListener.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
 
----------------
wolfgangp wrote:
> markoshorro wrote:
> > andreadb wrote:
> > > Is this because of clang-format? Otherwise please revert.
> > Yes, this was because of clang-format, should I revert it anyways?
> Yeah, generally we don't want to mix functional and (independent) formatting changes in the same patch. You're welcome to submit a second patch with just formatting changes, however.
Noted, good to know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105064