[PATCH] D93372: [CSKY 3/n] Add bare-bones C-SKY MCTargetDesc

[Zixuan Wu via Phabricator via llvm-commits]

zixuan-wu marked 2 inline comments as done.
zixuan-wu added inline comments.


================
Comment at: llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h:12
+
+#include "MCTargetDesc/CSKYMCTargetDesc.h"
+#include "llvm/MC/MCAsmBackend.h"
----------------
rengolin wrote:
> zixuan-wu wrote:
> > rengolin wrote:
> > > Do you really need this include here?
> > Yes, class MCTargetOptions is introduced by it.
> Why not include `"llvm/MC/MCTargetOptions.h"` directly here?
> 
> Your target description header has some auto-generated code that could get really large.
Right. I move it to related cpp file CSKYAsmBackend.cpp which needs that header file.


================
Comment at: llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.cpp:26
+
+template <int num>
+unsigned CSKYMCCodeEmitter::getOImmOpValue(const MCInst &MI, unsigned Idx,
----------------
myhsu wrote:
> 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?
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93372/new/

https://reviews.llvm.org/D93372