This is an archive of the discontinued LLVM Phabricator instance.

[CSKY] Complete to add basic integer instruction set
ClosedPublic

Authored by zixuan-wu on Oct 13 2021, 3:04 AM.

Details

Summary

Complete the basic integer instruction set and add related predictor in CSKY.td. And it includes the instruction definition and asm parser support. Following patches would be codegen pattern.

Diff Detail

Event Timeline

zixuan-wu created this revision.Oct 13 2021, 3:04 AM
zixuan-wu requested review of this revision.Oct 13 2021, 3:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2021, 3:04 AM

I have a few comments, but otherwise, it's looking good. Thanks!

llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
98

extra comment?

123

From To? To match the ones below?

221

This is getting too long, why can't you just use the template version directly?

282–338

Do you really need this templated version?

389

Shouldn't this be an error? Or an assert?

llvm/lib/Target/CSKY/CSKYInstrInfo.td
312

Sorry, I'm missing the context... Why isn't this a GPR? Is i32 a 32-bit view of the register (like AArch64)?

If so, then i32 is confusing because of the IR types. Perhaps GPR32?

zixuan-wu added inline comments.Oct 14 2021, 7:11 PM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
221

Because the interface is defined in generated inc file like isUImm1(), isUImm2()...

282–338

For now only used in isRegSeq(), but also used in float register sequence predictor isRegSeqF1() which is not upstreamed now.

389

I think it's a print result, it may not make sense to assert to debug.

llvm/lib/Target/CSKY/CSKYInstrInfo.td
312

because GPR is also for DSP instruction so that it can also contain v4i8 and v2i16 type.

zixuan-wu marked 2 inline comments as done and an inline comment as not done.Oct 15 2021, 2:49 AM
rengolin added inline comments.Oct 15 2021, 3:48 AM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
221

I feel like I've asked this before... :( Sorry. Ignore me.

282–338

Thought so, np.

389

But isn't this going to also print into asm files? If so, then you could in theory print out broken asm files and only notice because the assembler crashes.

llvm/lib/Target/CSKY/CSKYInstrInfo.td
312

From CSKYRegisterInfo.td, GPR lists only R* registers. Do those alias with DPS registers?

Other targets keep GPR as scalar registers and have other naming for registers that can alias FP/Vector registers.

For example, AArch64 names GPR64 and GPR32 (and many other classes) to deal with those issues.

You're better off naming the classes now and specialising later, than just use the bit-width. This makes it much simpler to read the table-gen files and also maintain them when they grow too large.

zixuan-wu added inline comments.Oct 15 2021, 4:13 AM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
389

It's not printed in asm file but in mir format in debug output.

llvm/lib/Target/CSKY/CSKYInstrInfo.td
312

If I understand correctly, you prefer to use another register class such as DSPR to represent v4i8 and v2i16 even if it also contains general register R0-R31 as GPR?

rengolin added inline comments.Oct 15 2021, 4:39 AM
llvm/lib/Target/CSKY/CSKYInstrInfo.td
312

Yes, that's the idea. Sorry for going around in circles. :)

zixuan-wu updated this revision to Diff 380279.Oct 17 2021, 9:24 PM
zixuan-wu marked an inline comment as done.

Is there anybody who has any more comments?

rengolin accepted this revision.Oct 18 2021, 2:00 AM

LGTM, thanks!

Just wait another day to make sure no one else has more questions.

This revision is now accepted and ready to land.Oct 18 2021, 2:00 AM
This revision was automatically updated to reflect the committed changes.