Based on the discussion in [1], minor changes have been done to properly output a valid JSON. Removed "not implemented" keys.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/docs/CommandGuide/llvm-mca.rst | ||
---|---|---|
214 | May I suggest another wording: The individual views refer to them by index. However, not all views are supported. Unsupported | |
215 | Don't we support the timeline view? | |
llvm/tools/llvm-mca/Views/TimelineView.h | ||
184 | Like in the comment above, I tought we implemented the timeline view. |
Fixed test, added JSON check using the same approach followed in [1], as @gbedwell suggested in [2].
[1] https://github.com/llvm/llvm-project/blob/20df2c7052c09934ce87ccc409da9d3dc24b7ca0/llvm/test/Other/new-pm-time-trace.ll#L5
[2] https://bugs.llvm.org/show_bug.cgi?id=50922#c2
Hi,
Are you sure that you have uploaded the full diff?
A lot of things seem to be missing from the patch. For example, I cannot see where you add the string "Instructions and CPU resources". But that is just an example.
I left a few comments below.
However, you should re-upload this patch with full context (using -U999999 has documented here: https://releases.llvm.org/11.0.0/docs/Phabricator.html#requesting-a-review-via-the-web-interface).
Thanks,
-Andrea
llvm/docs/CommandGuide/llvm-mca.rst | ||
---|---|---|
214–216 | "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." | |
992 | thanks for catching this. | |
llvm/test/tools/llvm-mca/JSON/X86/views.s | ||
57–79 | Why do we nest the name/value pairs for "Resources" and "Instructions" under "Instructions and CPU resources"? | |
llvm/tools/llvm-mca/Views/BottleneckAnalysis.h | ||
49–57 | Please revert this change. It is incorrect. The probability information should always be on the same line as the instruction. For the future: all these non-functional changes should be sent for review as a separate patch. They are not strictly related to what you are trying to fix. It just makes it harder to stay focused on the actual important changes. | |
llvm/tools/llvm-mca/Views/DispatchStatistics.cpp | ||
87–96 | Please add a test for this. | |
llvm/tools/llvm-mca/Views/TimelineView.h | ||
128 | Please revert. | |
llvm/tools/llvm-mca/Views/View.h | ||
19–22 | Is this because of clang-format? Otherwise please revert. |
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
StringRef getNameAsString() const override { return "Instructions and CPU resources"; }
llvm/docs/CommandGuide/llvm-mca.rst | ||
---|---|---|
215 | My bad, you are right. | |
llvm/tools/llvm-mca/Views/BottleneckAnalysis.h | ||
49–57 | You are right. | |
llvm/tools/llvm-mca/Views/DispatchStatistics.cpp | ||
87–96 | Sure, I forgot. | |
llvm/tools/llvm-mca/Views/View.h | ||
19–22 | Yes, this was because of clang-format, should I revert it anyways? |
llvm/test/tools/llvm-mca/JSON/X86/views.s | ||
---|---|---|
57–79 | 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? | |
llvm/tools/llvm-mca/Views/View.h | ||
19–22 | 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. |
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
llvm/test/tools/llvm-mca/JSON/X86/views.s | ||
---|---|---|
57–79 | Mm.. I see. In that case, I think it is fine. It may not be worthy to change things too much. |
@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.
llvm/docs/CommandGuide/llvm-mca.rst | ||
---|---|---|
214–216 | Noted. | |
llvm/test/tools/llvm-mca/JSON/X86/views.s | ||
57–79 | 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? | |
llvm/tools/llvm-mca/Views/View.h | ||
19–22 | Noted, good to know. |
@andreadb thanks for your guidance! I believe I do not have committ access, so I do not know if I have to ask for permission or if someone can do it on my behalf.
Again, thanks!
No problem,
I can commit this patch on your behalf.
Is it OK if I use your @udc.es email address, or do you prefer to use another email?
Thanks
-Andrea
Yes! marcos.horro@udc.es
Thanks! And just to know, what is it needed to get commit access? I guess more contributions, isn't it?
Have a look at this page: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
That link should hopefully answer to all your questions.
In my opinion, it makes sense to request for commit access if the plan is to keep contributing to the project.
My understanding is that it is mostly about how committed a developer is to the project, rather than how many commits have already been contributed.
But, that is just my personal interpretation of the process. As I wrote, you should be able to find "official" answers from that link :)
-Andrea
May I suggest another wording:
The individual views refer to them by index. However, not all views are supported. Unsupported
views are not printed, such as bottleneck analysis, etc.