Page MenuHomePhabricator

[llvm-tblgen][RISCV] Make llvm-tblgen RISCVCompressInstEmitter to be common infra across different targets
ClosedPublic

Authored by zixuan-wu on Tue, Nov 9, 2:27 AM.

Details

Summary

Not only RISCV but also other target such as CSKY, there are compressed instructions mixed with normal instructions. To reuse the basic infra to compress/uncompress and predict instruction, we need reconstruct the RISCVCompressInstEmitter and make it more general and suitable for other target.

This patch contains CSKY related part to show the whole picture after the tblgen tool changes, which make review better. It will be split to another patch when it is landed.

Diff Detail

Event Timeline

zixuan-wu created this revision.Tue, Nov 9, 2:27 AM
zixuan-wu requested review of this revision.Tue, Nov 9, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 9, 2:27 AM
zixuan-wu edited the summary of this revision. (Show Details)Tue, Nov 9, 2:28 AM
zixuan-wu updated this revision to Diff 386043.Tue, Nov 9, 7:12 PM
craig.topper added inline comments.
llvm/utils/TableGen/CompressInstEmitter.cpp
419

Are these errors the only reasons we need the Inst32 and Inst16 classes to be introduced in Target.td? Can we check the size relationships instead? If we're making this generic it doesn't seem like we should be hardcoding 16 and 32.

zixuan-wu added inline comments.Tue, Nov 9, 10:41 PM
llvm/utils/TableGen/CompressInstEmitter.cpp
419

Hmm. Good advice. I think checking size is better way to tell the instruction width.

zixuan-wu updated this revision to Diff 386401.Wed, Nov 10, 7:06 PM
zixuan-wu added a reviewer: rengolin.

Address comments.

jrtc27 added inline comments.Wed, Nov 10, 7:22 PM
llvm/utils/TableGen/CompressInstEmitter.cpp
1

Generator for Compression Inst doesn't make sense; Inst Compression does, or you could leave it as Generator for Compression given it used to be Generator for RISCV Compression

35

This used to say what the name of the header is

419

This still hard-codes 16 and 32 as compressed and uncompressed instruction sizes. When it was RISC-V-specific that was fine, but this is now a generic pass and, whilst the only consumers are architectures where those happen to be the two instruction widths involved, that won't necessarily be true of other architectures. As far as I can tell nothing in this generator cares what the sizes are; just change it to enforce that the compressed instruction is strictly smaller than the uncompressed instruction, i.e. it's actually compressing.

558

Namespace is not as informative, and does not make sense to prepend to "Subtarget". Call this TargetName instead (see the various methods in SubtargetFeatureInfo that take a TargetName and use it both as a prefix for Subtarget and as a namespace).

Address comments.

zixuan-wu marked 4 inline comments as done.Wed, Nov 10, 10:24 PM
luismarques accepted this revision.Thu, Nov 11, 7:36 AM

LGTM now. Thanks!

llvm/include/llvm/Target/Target.td
664–665

nit: remove the extraneous spaces. (I know it was already like this in the RISC-V version).

This revision is now accepted and ready to land.Thu, Nov 11, 7:36 AM
frasercrmck added inline comments.Thu, Nov 11, 7:48 AM
llvm/include/llvm/Target/Target.td
663

We should probably comment this class and its fields. At least direct users to the CompressInstEmitter backend.

llvm/utils/TableGen/CompressInstEmitter.cpp
27

isCompressOnly missing from this description.

96

I know you've inherited this from the RISCV version, but I'm not a fan of the same-line commenting: it messes up the formatting. Could they be on the lines above?

zixuan-wu updated this revision to Diff 386713.Thu, Nov 11, 7:03 PM

Address comments.

zixuan-wu updated this revision to Diff 386714.Thu, Nov 11, 7:04 PM
zixuan-wu marked 4 inline comments as done.

Is there anymore comments? I will hold this review for several days. If there is no more comments, I will commit it later since it's already accepted.

jrtc27 added inline comments.Sun, Nov 14, 6:29 PM
llvm/include/llvm/Target/Target.td
663–674

Most of these comments don't make sense to me. Especially isCompressOnly's.

zixuan-wu added inline comments.Sun, Nov 14, 7:23 PM
llvm/include/llvm/Target/Target.td
663–674

Anymore specific suggestion?

I would like to use another NFC commit to address comments issue. Commit this patch firstly.