[PATCH] D124826: [LoongArch] Add privilege instructions definition

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n added a comment.

I'm a bit busy doing other projects (and day job), but thanks for the patch; coverage should be pretty complete after this, and finally the meaty parts are coming!



================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.td:555
+//===----------------------------------------------------------------------===//
+// Privilege Instructions
+//===----------------------------------------------------------------------===//
----------------
nit: "Privileged" -- please fix all other occurrences of "privilege instruction(s)" everywhere else.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.td:558-567
+// CSR Access Instructions
+def CSRRD : FmtCSR<0b0000010000000, (outs GPR:$rd), (ins uimm14:$csr_num),
+                   "csrrd", "$rd, $csr_num">;
+let Constraints = "$rd = $dst" in {
+def CSRWR : FmtCSR<0b0000010000001, (outs GPR:$dst),
+                   (ins GPR:$rd, uimm14:$csr_num), "csrwr", "$rd, $csr_num">;
+def CSRXCHG : FmtCSRXCHG<0b00000100, (outs GPR:$dst),
----------------
So we don't treat `csrrd`and `csrwr` as special-cases of `csrxchg`, sharing the same opcode, only with different behavior? i.e., warn if `csrxchg`'s rj is r0 or r1, but don't outright fail, and make `csrrd` and `csrwr` syntactic sugars. This way `csrxchg`'s special semantics are left to consumers to handle, which I think would be acceptable. The benefit is that `csrxchg` remains as a "direct" representation of underlying machine code, without everyone having to deal with overlapping opcodes for only 3 exceedingly rare insns. No sane developer would regalloc r0 or r1 for an atomic CSR access after reading the manual, and if they really wanted to write a zero without using a temp register, a runtime error during testing is more-or-less the same as a compile-time error.

The current approach *may* be inconvenient, as I could envision some existing tools generating assembly code for llvm-mc, but unable to represent completely overlapping opcodes; these tools would only be able to emit plain `csrxchg` under the current implementation, as putting r0 or r1 into its rj slot would be illegal. I might be wrong here, so I'd welcome additional input.


================
Comment at: llvm/test/MC/LoongArch/Basic/Privilege/invalid.s:12
+iocsrrd.d $a0, $a1
+# ERR32: :[[#@LINE-1]]:1: error: instruction requires the following: LA64 Basic Integer and Privilege Instruction Set
+iocsrwr.d $a0, $a1
----------------
same as above: "privileged". Change wherever necessary for fixing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124826