Page MenuHomePhabricator

[RFC][Target] Add a new backend target called CSKY
Needs ReviewPublic

Authored by zixuan-wu on Aug 20 2020, 1:25 AM.

Details

Summary

C-SKY processor is a 32-bit high-performance and low-power embedded processor designed for embedded system or SoC environment. It adopts independently design of architecture and micro architecture with extensible instruction set, which owns great features, e.g. configurable hardware, re-synthesis, easily integration etc. Additionally, it is excellent in power management. It adopts several strategies to reduce power consumption including statically designed and dynamic power supply management, low voltage supply, entering low power mode and closing internal function modules.

Now, C-SKY CPU instruction system has two versions: C-SKY V1 and C-SKY V2. You must note that the C-SKY V2.0 instruction sets are not freely exchangeable with V1.0. Conversely, available function provided by V2.0 is identical to V1.0 for most of applications. So that we strongly recommend that you should make sure you are aware of the generated result of specified application when you use them simultaneously. The two instruction sets differ in how instructions are encoded.

Basically, we can develop C-SKY V2 as prefer, and the ISA doc and ABI doc can be referred at following links. (The links are not stable, and the address can be changed)

ISA: https://github.com/c-sky/csky-doc/blob/master/CSKY%20Architecture%20user_guide.pdf
ABI: https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf

This is the first patch to introduce the CSKY target and of course it's experimental now. Here is the RFC proposal mail: http://lists.llvm.org/pipermail/llvm-dev/2020-August/144481.html

Diff Detail

Event Timeline

zixuan-wu created this revision.Aug 20 2020, 1:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2020, 1:25 AM
zixuan-wu requested review of this revision.Aug 20 2020, 1:25 AM
zixuan-wu edited the summary of this revision. (Show Details)Aug 20 2020, 7:19 PM
zixuan-wu added reviewers: efriedma, arsenm.
arsenm added inline comments.Aug 21 2020, 6:52 AM
llvm/include/llvm/ADT/Triple.h
59 ↗(On Diff #286730)

Usually the triple patch is split into a separate patch

llvm/lib/Target/CSKY/CSKYInstrInfo.cpp
83

Grammatically weird error throughout

llvm/lib/Target/CSKY/CSKYMachineFunctionInfo.h
36

I'm trying to remove the dependency on machine function state in the constructor in D80249

llvm/lib/Target/CSKY/CSKYRegisterInfo.cpp
74

Can do just MI->getOpcode()

79

Register

150

Constant if

llvm/lib/Target/CSKY/CSKYSubtarget.cpp
46–48

No reason to use std:: string here

llvm/lib/Target/CSKY/CSKYTargetMachine.cpp
99–117

We should move this logic into the base implementation with a template argument for the subtarget class; I think every target reproduces approximately the same thing

Thank you for your review, arsenm!

llvm/lib/Target/CSKY/CSKYMachineFunctionInfo.h
36

D80249 has not been committed yet. I think I can rebase and catch up D80249 if it's committed before I commit this patch. If not, I would keep machine function state as original.

llvm/lib/Target/CSKY/CSKYTargetMachine.cpp
99–117

Yes, it's too much housekeeping work. But it's another orthogonal issue and should be handled together with other targets in other patch.

zixuan-wu marked 5 inline comments as done.Aug 24 2020, 9:15 PM
zixuan-wu added inline comments.
llvm/include/llvm/ADT/Triple.h
59 ↗(On Diff #286730)
zixuan-wu updated this revision to Diff 287553.EditedAug 24 2020, 9:22 PM

Address comments. Split triple work as separate part https://reviews.llvm.org/D86505.

zixuan-wu updated this revision to Diff 287923.EditedAug 26 2020, 4:39 AM

take apart ELF related logics into separate patch, D86610.

gentle ping...

gentle ping...

craig.topper added inline comments.Sep 13 2020, 12:49 PM
llvm/include/llvm/IR/CMakeLists.txt
18

I believe this list is in alphabetical order. So CSKY should be between BBF and Hexagon

llvm/include/llvm/IR/IntrinsicsCSKY.td
2

Is this line longer than 80 characters? It looks longer than line 8 which I assume is 80 characters.

llvm/lib/Target/CSKY/CSKYAsmPrinter.h
2

Can we shorten this line to avoid wrapping?

I think the -*- C++ -*- part needs to stay together.

llvm/lib/Target/CSKY/CSKYCallingConv.h
2

Avoid wrapping this

llvm/lib/Target/CSKY/CSKYConstantPoolValue.cpp
2

Pad to 80 columns

llvm/lib/Target/CSKY/CSKYConstantPoolValue.h
2

Pad to 80 columns

llvm/lib/Target/CSKY/CSKYFrameLowering.cpp
28

Capitalize variable name

llvm/lib/Target/CSKY/CSKYISelDAGToDAG.cpp
95

I believe LLVM style would prefer "auto *SDIVREM" over "auto SDIVREM" so it;s clear to the read it is a pointer. This applies to other places in this file.

llvm/lib/Target/CSKY/CSKYISelLowering.cpp
30

Use MCPhysReg instead of uint16_t.

159

Stray semicolon?

242

Is this commented out code needed?

388

Is this code needed?

490

Is this code needed?

llvm/lib/Target/CSKY/CSKYInstrFormatsF1.td
2

80 columns

llvm/lib/Target/CSKY/CSKYInstrInfoF1.td
2

Pad to 80 columns

llvm/lib/Target/CSKY/CSKYTargetMachine.cpp
102

Replace this with

std::string CPU = CPUAttr.isValid() ? CPUAttr.getValueAsString().str() : TargetCPU;

Same for FSAttr. This change made was to all targets in tree a few weeks ago.

llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCExpr.cpp
59

Is this code needed?

Address comments.

zixuan-wu marked 17 inline comments as done.Sep 25 2020, 12:54 AM

Thank you so much @craig.topper for your detail review comments.

zixuan-wu updated this revision to Diff 294251.Sep 25 2020, 1:53 AM
zixuan-wu added a comment.EditedSep 28 2020, 2:09 AM

Ping... :)

This is a massive code drop; I don't think it will be possible for any one person to review.

llvm/lib/Target/CSKY/CSKY.h
28

Give the parameter a name? Looks like it's used.

llvm/lib/Target/CSKY/CSKYAsmPrinter.cpp
85–86

Do you plan to add more cases or can this just be a simple if statement for now?

llvm/lib/Target/CSKY/CSKYConstantIslandPass.cpp
284

name param

384

?

545

would a range-for work better here? (similar to what's used on L552)

616

I see MI.getOperand(Op) subexpression repeated. I think we have a range-for over operands that might help here?

This patch is too big to review. I will imitate the RISCV upstream process like D23560 to split this patch.

First one is https://reviews.llvm.org/D88466

lattner resigned from this revision.Sep 28 2020, 10:32 PM