This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] Add per BB instruction mix remark.
ClosedPublic

Authored by fhahn on Oct 21 2020, 9:02 AM.

Details

Summary

This patch adds a remarks that provides counts for each opcode per basic block.

An snippet of the generated information can be seen below.

The current implementation uses the target specific opcode for the counts. For example, on AArch64 this means we currently get 2 entries for add instructions if the block contains 32 and 64 bit adds. Similarly, immediate version are treated differently.

Unfortunately there seems to be no convenient way to get only the mnemonic part of the instruction as a string AFAIK. This could be improved in the future.

--- !Analysis
Pass:            asm-printer
Name:            InstructionMix
DebugLoc:        { File: arm64-instruction-mix-remarks.ll, Line: 30, Column: 30 }
Function:        foo
Args:
  - String:          'BasicBlock: '
  - BasicBlock:      else
  - String:          "\n"
  - String:          INST_MADDWrrr
  - String:          ': '
  - INST_MADDWrrr:   '2'
  - String:          "\n"
  - String:          INST_MOVZWi
  - String:          ': '
  - INST_MOVZWi:     '1'

Diff Detail

Event Timeline

fhahn created this revision.Oct 21 2020, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2020, 9:02 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn requested review of this revision.Oct 21 2020, 9:02 AM
fhahn edited the summary of this revision. (Show Details)Oct 21 2020, 11:10 AM
fhahn added reviewers: anemet, thegameg, paquette, efriedma.
fhahn updated this revision to Diff 299756.Oct 21 2020, 11:12 AM

Sort entries by count and name.

If there's a convenient way to get only the mnemonic part of the opcode name, that would be really helpful to know :)

anemet accepted this revision.Oct 21 2020, 11:28 AM
anemet added a subscriber: aemerson.

LGTM, you may want to wait a little for other potential comments. Especially some ideas on getting the actual mnemonic.

This revision is now accepted and ready to land.Oct 21 2020, 11:28 AM
paquette added inline comments.Oct 21 2020, 3:30 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1207

Will this work with inline assembly? Looks like inline assembly is handled earlier in the switch.

(Probably fine to support it later anyway.)

1255

MBB.empty()?

1265

Is there any reason that you're prefixing these with INST_? Are you planning on adding other ways to categorise these?

For the mnemonics, it looks like the logic to get at them is currently table-generated into build/lib/Target/MyTarget/MyTargetGenAsmWriter.inc.

I suppose you could factor out the logic for mnemonic printing in utils/TableGen/AsmWriterEmitter.cpp into something that can also be used here? Not sure if there is any better way than that...

thegameg accepted this revision.Oct 22 2020, 8:25 PM

This looks great! Thanks!

If this evolves into something bigger (e.g. per-loop instruction mix remarks, instruction mix in assembly comments, etc.) it may be useful to have it around as an analysis.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1125

Looks like we can have loop info around here too, maybe a nice future extension would be to emit a remark with the instruction mix per loop too.

How about adding a total count as well? That should help quickly eye ball the ratios.

How about adding a total count as well? That should help quickly eye ball the ratios.

Wouldn't that overlap with the InstructionCount remark?

fhahn updated this revision to Diff 300264.Oct 23 2020, 6:26 AM
fhahn marked an inline comment as done.

Thanks for all the comments. Updated to use MBB.empty().

How about adding a total count as well? That should help quickly eye ball the ratios.

Wouldn't that overlap with the InstructionCount remark?

I think so. Or do you mean per instruction counts for the whole function? That could also be done by post-processing I think.

fhahn added inline comments.Oct 23 2020, 6:30 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1125

Sounds like a useful extension in the future indeed!

1207

Hmm, I am not sure we can really do much about inline assembly in general. Isn't that just a opaque string passed through?

1265

The prefix is intended to make it easier for tool to post-process the information to disambiguate the counts from other info (like the BB). But at the moment it is not really necessary and mostly for convenience. For now I am not planning on adding other ways to categorize the instruction counts. That seems tricky to do here in a target-independent way unfortunately.

fhahn added a comment.Oct 23 2020, 6:40 AM

For the mnemonics, it looks like the logic to get at them is currently table-generated into build/lib/Target/MyTarget/MyTargetGenAsmWriter.inc.

I suppose you could factor out the logic for mnemonic printing in utils/TableGen/AsmWriterEmitter.cpp into something that can also be used here? Not sure if there is any better way than that...

I tried to do that as follow up in D90039 & D90040. Seems to work reasonably well, although it seems like it misses the conditional codes for branches.

paquette accepted this revision.Oct 23 2020, 10:06 AM

LGTM!

The follow-up changes look really nice too.

This revision was landed with ongoing or failed builds.Oct 26 2020, 2:26 AM
This revision was automatically updated to reflect the committed changes.