[PATCH] D111701: [CSKY] Complete to add basic integer instruction set

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:222
 
+  bool isUImm1() const { return isUImm<1>(); }
   bool isUImm2() const { return isUImm<2>(); }
----------------
zixuan-wu wrote:
> rengolin wrote:
> > This is getting too long, why can't you just use the template version directly?
> Because the interface is defined in generated inc file like isUImm1(), isUImm2()...
I feel like I've asked this before... :( Sorry. Ignore me.


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:283
 
-  bool isConstpoolSymbol() const {
+  template <unsigned MIN, unsigned MAX> bool isRegSeqTemplate() const {
+    if (!isRegisterSeq())
----------------
zixuan-wu wrote:
> rengolin wrote:
> > Do you really need this templated version?
> For now only used in isRegSeq(), but also used in float register sequence predictor isRegSeqF1() which is not upstreamed now.
Thought so, np.


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:399
+      else
+        return "noreg";
+    };
----------------
zixuan-wu wrote:
> rengolin wrote:
> > Shouldn't this be an error? Or an assert?
> I think it's a print result, it may not make sense to assert to debug.
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.


================
Comment at: llvm/lib/Target/CSKY/CSKYInstrInfo.td:312
   def ORI32 : I_16_ZX<"ori32", uimm16,
-  [(set GPR:$rz, (or GPR:$rx, uimm16:$imm16))]>;
+  [(set i32:$rz, (or i32:$rx, uimm16:$imm16))]>;
   def XORI32 : I_12<0x4, "xori32", xor, uimm12>;
----------------
zixuan-wu wrote:
> rengolin wrote:
> > 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`?
> because GPR is also for DSP instruction so that it can also contain v4i8 and v2i16 type.
>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.


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

https://reviews.llvm.org/D111701