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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h | ||
---|---|---|
13 | Yes, class MCTargetOptions is introduced by it. |
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. |
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? |
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. |
llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.cpp | ||
---|---|---|
38 | No need to define since it has a default definition. | |
llvm/lib/Target/CSKY/MCTargetDesc/CSKYELFObjectWriter.cpp | ||
37 | Add a period for complete sentences in comments | |
llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.cpp | ||
23 | The generic name can be misleading. Add a target prefix | |
llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.h | ||
46 | clang-format this line |
llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.h | ||
---|---|---|
46 | It's been clang-formatted. |
Do you really need this include here?