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

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

xen0n added a comment.

Now that my concern is more-or-less addressed, let's see what others think...



================
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),
----------------
SixWeining wrote:
> 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.
Looking at the manual more closely, it actually didn't mention the requirement to treat `csrrd/csrwr` as distinct from `csrxchg`; only in the Appendix B (instruction encodings) you can see the distinction. Again here it's arguably obvious the three insns are sharing the same opcode prefix (thus implying `csrrd/csrwr` are rational abuses of `csrxchg` encoding).

For now I agree with you though; on one hand I didn't check the binutils behavior, on the other hand a more direct syntax (and other amendments to the manual) would be better for a future iteration, both for projects (binutils/llvm/etc.) and something like a v1.10 or even v2.00 manual.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124826