This is an archive of the discontinued LLVM Phabricator instance.

[CSKY 3/n] Add bare-bones C-SKY MCTargetDesc
ClosedPublic

Authored by zixuan-wu on Dec 15 2020, 10:50 PM.

Details

Summary

Add basis of CSKY MCTargetDesc and it's enough to compile and link but doesn't yet do anything particularly useful.
Once an ASM parser and printer are added in the next two patches, the whole thing can be usefully tested.

Diff Detail

Event Timeline

zixuan-wu created this revision.Dec 15 2020, 10:50 PM
zixuan-wu requested review of this revision.Dec 15 2020, 10:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 10:50 PM

Looks like a standard mock implementation, basic infrastructure. I don't see anything wrong with it but I've also been far from this area for too long, so I'll let other people review it properly and approve.

llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h
13

Do you really need this include here?

zixuan-wu added inline comments.Dec 17 2020, 12:10 AM
llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h
13

Yes, class MCTargetOptions is introduced by it.

zixuan-wu marked an inline comment as done.Dec 17 2020, 12:10 AM
rengolin added inline comments.Dec 17 2020, 3:21 AM
llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h
13

Why not include "llvm/MC/MCTargetOptions.h" directly here?

Your target description header has some auto-generated code that could get really large.

myhsu added inline comments.Dec 17 2020, 9:36 AM
llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.cpp
27

Are getOImmOpValue and getImmOpValue only gonna used in this file? Because if they're used in other files, there will be a linker error saying that it can't find the template-instantiated version of these two functions (unless you explicitly instantiate every possible template arguments here). And I think in general you should avoid splitting template definitions from its declarations.

And also, why num is not used in both functions?

zixuan-wu marked 2 inline comments as done.Dec 17 2020, 7:25 PM
zixuan-wu added inline comments.
llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h
13

Right. I move it to related cpp file CSKYAsmBackend.cpp which needs that header file.

llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.cpp
27

Good. I think the num is not needed anymore.

Second, I think I can write template implementation in header in short version now. In future, the complex body of getImmOpValue can be departed into another function and its implementation is in cpp file.

zixuan-wu updated this revision to Diff 312669.Dec 17 2020, 7:27 PM
zixuan-wu marked 2 inline comments as done.

Address comments.

Is it good to anybody to LGTM?

rengolin accepted this revision.Dec 21 2020, 1:01 PM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 21 2020, 1:01 PM
MaskRay added inline comments.Dec 21 2020, 4:35 PM
llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.cpp
76

No newline at end of file

llvm/lib/Target/CSKY/MCTargetDesc/CSKYELFObjectWriter.cpp
14

You may use csky-elf-writer or csky-elf-object-writer

The current generic name can be misleading.

MaskRay added inline comments.Dec 21 2020, 4:40 PM
llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.cpp
37

No need to define since it has a default definition.

llvm/lib/Target/CSKY/MCTargetDesc/CSKYELFObjectWriter.cpp
36

Add a period for complete sentences in comments

llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.cpp
22

The generic name can be misleading. Add a target prefix

llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.h
45

clang-format this line

zixuan-wu updated this revision to Diff 313228.Dec 21 2020, 6:48 PM

Address comments.

zixuan-wu marked 5 inline comments as done.Dec 21 2020, 6:49 PM
zixuan-wu added inline comments.
llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.h
45

It's been clang-formatted.

This revision was automatically updated to reflect the committed changes.