This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Collect Statistics on Compressed Instructions
ClosedPublic

Authored by lenary on Sep 12 2019, 5:51 AM.

Details

Summary

It is useful to keep statistics on how many instructions we have
compressed, so we can see if future changes are increasing or decreasing this
number.

Event Timeline

lenary created this revision.Sep 12 2019, 5:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 5:51 AM
luismarques accepted this revision.Jan 2 2020, 2:18 AM

LGTM. My only concern was if it made sense to use the same statistic to count in both places, and if we could end up double counting the instructions emitted (now, or in a future LLVM version). After a quick look I didn't really see other targets using the same approach, but I also can't think of a way where this ends up actually being problematic.

(Nipick: maybe put a comment in the statistic reminding people that the linker may further compress some of the instructions we emitted uncompressed?)

This revision is now accepted and ready to land.Jan 2 2020, 2:18 AM
xbolva00 added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1618–1621

under if (Res) too ?

lenary marked an inline comment as done.Jan 6 2020, 7:07 AM
lenary added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1618–1621

This was covered in rGa0f43b0043581b37b10d105a85f0653704d3657b - I just need to rebase this patch.

lenary added a comment.Jan 9 2020, 3:40 AM

LGTM. My only concern was if it made sense to use the same statistic to count in both places, and if we could end up double counting the instructions emitted (now, or in a future LLVM version). After a quick look I didn't really see other targets using the same approach, but I also can't think of a way where this ends up actually being problematic.

My understanding is that instructions only go through one of these functions. RISCVAsmParser.cpp is used by the assembler only, and the RISCVAsmPrinter.cpp is only used by LLVM CodeGen. This should mean that instructions are not double-counted (today). Yes I'm not sure why we have both, I think @asb did this to ensure better layering.

Note, the statistics are not the same. Statistics are namespaced by DEBUG_TYPE, which is why I had to define it in RISCVAsmParser.cpp.

(Nipick: maybe put a comment in the statistic reminding people that the linker may further compress some of the instructions we emitted uncompressed?)

The point in these statistics is to track how good LLVM is at compressing instructions (which may change depending on optimisation heuristics we implement eventually). I don't see this as being used by end users much, if at all.

asb added a comment.Jan 9 2020, 6:23 AM

LGTM. My only concern was if it made sense to use the same statistic to count in both places, and if we could end up double counting the instructions emitted (now, or in a future LLVM version). After a quick look I didn't really see other targets using the same approach, but I also can't think of a way where this ends up actually being problematic.

My understanding is that instructions only go through one of these functions. RISCVAsmParser.cpp is used by the assembler only, and the RISCVAsmPrinter.cpp is only used by LLVM CodeGen. This should mean that instructions are not double-counted (today). Yes I'm not sure why we have both, I think @asb did this to ensure better layering.

Both are necessary for completeness, and are independent paths for MCInst generation. We did go back and forth a fair bit on exactly where the compression hook should be called and that seemed the most sensible option - I forget the full analysis, but there wasn't a single place we could stick it while remaining localised to the RISC-V backend.

asb accepted this revision.Jan 9 2020, 6:23 AM
This revision was automatically updated to reflect the committed changes.