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

[Lu Weining via Phabricator via llvm-commits]

SixWeining added inline comments.


================
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),
----------------
xen0n wrote:
> SixWeining wrote:
> > xen0n wrote:
> > > 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.
> > Sorry I don't understand what you mean.
> > 
> > Do you mean the assembler can take `csrxchg rd, r0, num` as `csrrd rd`, and `csrxchg rd, r1, num` as `csrwr`?
> > Do you mean only need to define `csrxchg` and 2 alias (`csrrd` `csrrw`) for it? Just like `nop` is the alias of `andi r0, r0, 0`?
> > Sorry I don't understand what you mean.
> > 
> > Do you mean the assembler can take `csrxchg rd, r0, num` as `csrrd rd`, and `csrxchg rd, r1, num` as `csrwr`?
> > Do you mean only need to define `csrxchg` and 2 alias (`csrrd` `csrrw`) for it? Just like `nop` is the alias of `andi r0, r0, 0`?
> 
> Yes, exactly this.
OK, I see. Thanks for the suggestion.

I think this is more or less a modification request to the published [[ https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#csr-access-instructions | LoongArch ISA manual ]]. Until then let's keep the current approach which is loyal to the manual directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124826