Page MenuHomePhabricator

[MCA] Add flag -show-encoding to llvm-mca.
ClosedPublic

Authored by andreadb on Aug 8 2019, 6:28 AM.

Details

Summary

Flag -show-encoding enables the printing of instruction encodings as part of the the instruction info view.

Example (with flags -mtriple=x86_64-- -mcpu=btver2):

Instruction Info:
[1]: #uOps
[2]: Latency
[3]: RThroughput
[4]: MayLoad
[5]: MayStore
[6]: HasSideEffects (U)
[7]: Encoding Size

[1]    [2]    [3]    [4]    [5]    [6]    [7]    Encodings:                    Instructions:
 1      2     1.00                         4     c5 f0 59 d0                   vmulps   %xmm0, %xmm1, %xmm2
 1      4     1.00                         4     c5 eb 7c da                   vhaddps  %xmm2, %xmm2, %xmm3
 1      4     1.00                         4     c5 e3 7c e3                   vhaddps  %xmm3, %xmm3, %xmm4

In this example, column Encoding Size is the size in bytes of the instruction encoding.
Column Encodings reports the actual instruction encodings as byte sequences in hex (objdump style).

The computation of encodings is done by a utility class named mca::CodeEmitter.

In future, I plan to expose the CodeEmitter to the instruction builder, so that information about instruction encoding sizes can be used by the simulator. That would be a first step towards simulating the throughput from the decoders in the hardware frontend.

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb created this revision.Aug 8 2019, 6:28 AM
gchatelet accepted this revision.Aug 8 2019, 6:44 AM

Nice!
Please wait for other reviewers to have a look.

lib/MCA/CodeEmitter.cpp
20 ↗(On Diff #214134)

I'm not a huge fan of using the address of MCInst as the key in the cache, for instance the following code would be buggy:

CodeEmitter CE;
MCInst Inst = GetInst();
CE.getOrCreateEncodingInfo(Inst);
Inst = GetAnotherInst();
CE.getOrCreateEncodingInfo(Inst);

If you still want to use the address for performance reasons you can pass by pointer instead of reference. At least this will be a red flag,

This revision is now accepted and ready to land.Aug 8 2019, 6:44 AM

Docs missing.

andreadb marked an inline comment as done.Aug 8 2019, 8:05 AM

Docs missing.

I will add documentation for it.

lib/MCA/CodeEmitter.cpp
20 ↗(On Diff #214134)

Yes it would be bugged for that case.

However, that case is not going to happen in MCA because the sequence of MCInst is constructed once at the beginning, and then it is never changed. It is passed around as an ArrayRef and no module is meant to change any MCInsts or add extra elements to the sequence.
That sequence is never invalidated nor changed during the entire execution.

So yes, I totally agree with you that using a pointer is ugly and scary. But in practice - for our particular scenario - it works in a fast and easy.

That being said, I could use an instruction index.
However, that would complicate future patches to the InstrBuilder (which doesn't know anything about the instruction index). I'll see if I can use the instruction index and avoid any controversial change :).

+1 for the update to the llvm-mca.rst

tools/llvm-mca/llvm-mca.cpp
290 ↗(On Diff #214134)

Add this back.

andreadb updated this revision to Diff 214171.Aug 8 2019, 9:19 AM

Address review comments.

Added documentation for the new flag.

Changed how the CodeEmitter works.
Class CodeEmitter now uses instruction identifiers to reference MCInsts of a sequence.
It no longer uses a map. It uses a vector instead, and the instruction index is used to address that vector.

Is it already transitively enabled by -all-stats/-all-views, i think not?

Is it already transitively enabled by -all-stats/-all-views, i think not?

No it is not.

Is it already transitively enabled by -all-stats/-all-views, i think not?

No it is not.

TBH I'd prefer that this is disabled by default - it bulks out the instruction view quite a bit

Is it already transitively enabled by -all-stats/-all-views, i think not?

No it is not.

TBH I'd prefer that this is disabled by default - it bulks out the instruction view quite a bit

By default - i totally agree. But not by an explicit "please show me everything; no but really please do".

Is it already transitively enabled by -all-stats/-all-views, i think not?

No it is not.

TBH I'd prefer that this is disabled by default - it bulks out the instruction view quite a bit

By default - i totally agree. But not by an explicit "please show me everything; no but really please do".

To me, "enable all views" doesn't mean "show me everything". It literally means that all the views should contribute to the final report.
Extra flags like -show-encoding could then be used to further customize the output.
This is at least how I see it.

Is it already transitively enabled by -all-stats/-all-views, i think not?

No it is not.

TBH I'd prefer that this is disabled by default - it bulks out the instruction view quite a bit

By default - i totally agree. But not by an explicit "please show me everything; no but really please do".

To me, "enable all views" doesn't mean "show me everything". It literally means that all the views should contribute to the final report.
Extra flags like -show-encoding could then be used to further customize the output.
This is at least how I see it.

No strong opinion in this case, could potentially be changed later.
LG

gchatelet accepted this revision.Aug 9 2019, 1:40 AM
gchatelet added inline comments.
lib/MCA/CodeEmitter.cpp
20 ↗(On Diff #214134)

It looks much better now IMHO : ) Thx for taking the time to improve on it.

Thanks Guillaume.

I agree that it is better this way.

Closed by commit rL368432: [MCA] Add flag -show-encoding to llvm-mca. (authored by adibiagio, committed by ). · Explain WhyAug 9 2019, 4:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 4:26 AM