[PATCH] D105064: [llvm-mca] Fix JSON output
Andrea Di Biagio via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 29 11:45:35 PDT 2021
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
More information about the llvm-commits
mailing list