This is an archive of the discontinued LLVM Phabricator instance.

[CSKY] Add CSKY 16-bit instruction format and encoding
ClosedPublic

Authored by zixuan-wu on Nov 1 2021, 3:22 AM.

Details

Summary

CSKY is a ARCH which supports mixture of 16-bit and 32-bit instructions natively, and there is not an indivual predictor or feature to enable/disable 16-bit instruction. So I think it's better to add 16-bit instruction early, and naturally to use 16-bit and 32-bit instructions.

Diff Detail

Event Timeline

zixuan-wu created this revision.Nov 1 2021, 3:22 AM
zixuan-wu requested review of this revision.Nov 1 2021, 3:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 3:22 AM
rengolin added inline comments.Nov 1 2021, 11:02 AM
llvm/lib/Target/CSKY/CSKY.td
28

Huh! I haven't found any reference to Java instructions in either available manuals (in github).

I'm guessing this is similar to Arm's Jazelle extensions?

None of the instructions below marked with HasJAVA appear on the ISA document...

Perhaps better to leave it out for another commit once that part is public?

llvm/lib/Target/CSKY/CSKYInstrInfo16Instr.td
56

It's not necessarily mandatory, but generally useful to have sub-divisions on the TD file itself for instructions of different type.

In the ISA document, the instructions are separated by classes (arithmetic, logic, shift, etc). If they follow the same pattern here, it would be a lot easier to follow (and find) the implementation of each instruction.

llvm/lib/Target/CSKY/CSKYRegisterInfo.td
156

Would be nice to have a one-line description for the difference between GPR, sGPR and mGPR, or at least their purpose in instruction use, for example, "only registers that 16-bit instruction can access".

The other existing cases are more obvious as to what they are.

zixuan-wu added inline comments.Nov 2 2021, 2:18 AM
llvm/lib/Target/CSKY/CSKY.td
28

for example, BPOPH has reference to it.

zixuan-wu updated this revision to Diff 384009.Nov 2 2021, 2:19 AM

Address comments.

rengolin added inline comments.Nov 2 2021, 4:03 AM
llvm/lib/Target/CSKY/CSKY.td
28

The documents you posted on the RFC (https://github.com/c-sky/csky-doc/) don't have it.

It has reference to bpop16.h and bpop16.w in section 5.3.2.5, but does not list it in the instruction section, so we're left wondering what that is.

zixuan-wu added inline comments.Nov 2 2021, 7:07 PM
llvm/lib/Target/CSKY/CSKY.td
28

Oh, it's missing instructions in that doc, which may because of out-of-date.

rengolin added inline comments.Nov 3 2021, 2:36 AM
llvm/lib/Target/CSKY/CSKY.td
28

Can you update that document before updating the code with non-public information?

zixuan-wu marked an inline comment as done.Nov 4 2021, 2:14 AM
zixuan-wu added inline comments.
llvm/lib/Target/CSKY/CSKY.td
28

OK. I have updated the pdf document.

rengolin accepted this revision.Nov 4 2021, 5:11 AM

LGTM, thanks!

llvm/lib/Target/CSKY/CSKY.td
28

Thanks! It doesn't mention "java" but at least the instructions are there.

This revision is now accepted and ready to land.Nov 4 2021, 5:11 AM
zixuan-wu marked an inline comment as done.Nov 4 2021, 7:05 PM
zixuan-wu added inline comments.
llvm/lib/Target/CSKY/CSKY.td
28

No, all specific predictors are not listed explicitly in ISA.

This revision was automatically updated to reflect the committed changes.