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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In D105064#2847881 <https://reviews.llvm.org/D105064#2847881>, @markoshorro wrote:

> Thanks, @andreadb for the corrections. This is my first patch so I am sorry for these mistakes. I have left some comments inlined, however:
>
> - String "Instructions and CPU resources" was already there, under Views/InstructionView.h

Sorry if I came across a bit too dry/direct.

In reality, I am very happy that you have sent this patch!

And yes, you are right that the string was already there. My mistake.

Speaking about your code comment fixes: most of them are actually good changes.
However - as Wolfgang also pointed out - it would be better if you could send them as a separate (follow-up) patch.

Thanks



================
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:     ],
----------------
wolfgangp wrote:
> 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.
> > 
> > 
> I had put that in originally, so the instruction and resource list is treated like a view in its own right (not that it is one, just from a design point of view). Perhaps it needs to be redesigned?
Mm.. I see. In that case, I think it is fine. It may not be worthy to change things too much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105064