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

[Zixuan Wu via Phabricator via llvm-commits]

zixuan-wu added inline comments.


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:399
+      else
+        return "noreg";
+    };
----------------
rengolin wrote:
> 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.
It's not printed in asm file but in mir format in debug output.


================
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>;
----------------
rengolin wrote:
> 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.
If I understand correctly, you prefer to use another register class such as DSPR to represent v4i8 and v2i16 even if it also contains general register R0-R31 as GPR?


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

https://reviews.llvm.org/D111701