Page MenuHomePhabricator

Add an interface emitPrefix for MCCodeEmitter
ClosedPublic

Authored by skan on Dec 31 2019, 9:06 PM.

Details

Summary

To support for prefix padding to align branches within 32-Byte boundary D70157, we need to know the number of the existing prefixes of instructions , since too many prefixes before one instruction can be a performance issue on some microarchitecture.

The code of emitting prefixes is extracted out from the function encodeInstruction as emitPrefix, then we can emit the prefixes into a dummy stream, and count the prefixes of each instruction.

Diff Detail

Event Timeline

skan created this revision.Dec 31 2019, 9:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 31 2019, 9:06 PM

This description needs more context about what the end goal of this is for. Prefixes are a very X86 specific concept. Most of the targets in tree use fixed instruction widths.

Can we move the X86MCCodeEmitter definition to a header file and create a separate X86MCCodeEmitter belonging to the X86AsmBackend.cpp that that we can call the emitPrefix on? I'm not sure we want to expose the emitPrefix interface on the generic MCCodeEmitter interface.

skan edited the summary of this revision. (Show Details)Dec 31 2019, 9:51 PM
skan added a comment.Dec 31 2019, 10:18 PM

Can we move the X86MCCodeEmitter definition to a header file and create a separate X86MCCodeEmitter belonging to the X86AsmBackend.cpp that that we can call the emitPrefix on? I'm not sure we want to expose the emitPrefix interface on the generic MCCodeEmitter interface.

Currently, MCObjectStreamer owns a MCAssembler ,which owns a MCAsmBackend and a MCCodeEmitter, so I don't think making X86AsmBackend have a X86MCCodeEmitter object member is a good design. Currently, I plan to call the emitPrefix as OS.getAssembler().getEmitter().emitPrefix() in X86AsmBackend.cpp, where OS is a MCObjectStreamer object.

My thought was just that the encoder only has 2 member variables in it so its quite small to create. And it would avoid exposing the concept of prefixes to the MCCodeEmitter interface. But lets see what other reviewers think.

My thought was just that the encoder only has 2 member variables in it so its quite small to create. And it would avoid exposing the concept of prefixes to the MCCodeEmitter interface. But lets see what other reviewers think.

But I guess we don't have a MCContext object in X86AsmBackend so we can't run the X86MCCodeEmitter constructor so maybe that doesn't work at all.

skan edited the summary of this revision. (Show Details)Jan 1 2020, 11:21 PM
skan added a reviewer: MaskRay.Jan 2 2020, 2:03 AM
skan updated this revision to Diff 236234.Jan 5 2020, 4:19 AM

Rebase

craig.topper added inline comments.Jan 5 2020, 9:10 PM
llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
1365

When would emitPrefix be called on a X86II::Pseudo?

1419–1422

Why is Pseudo no longer in this switch?

skan marked 2 inline comments as done.Jan 5 2020, 11:02 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
1365

I don't know, encodeInstruction checks (TSFlags & X86II::FormMask) == X86II::Pseudo, so I do the same for emitPrefix. My thought is emitPrefix can be used anywhere where encodeInstruction can be used.

1419–1422

encodeInstruction always calls emitPrefixImpl, which already checks
Pseudo in the corresponding switch.

craig.topper added inline comments.Jan 5 2020, 11:57 PM
llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
1365

Ok thanks. I missed that.

1419–1422

Can you put it back just so that all possible values are mentioned by name in the switch. I think you can remove it from the emitPrefixImpl switch and just let it go to the default case.

skan updated this revision to Diff 236309.Jan 6 2020, 1:05 AM
skan marked an inline comment as done.
This revision is now accepted and ready to land.Jan 6 2020, 1:14 AM
This revision was automatically updated to reflect the committed changes.