[PATCH] D98936: [RISCV] DAG nodes and pseudo instructions for CSR access

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1168
+    PseudoInstExpansion<(CSRRC GPR:$rd, SR.Encoding, X0)> {
+  let hasSideEffects = 0;
+  let Uses = Regs;
----------------
evandro wrote:
> Methinks that `hasSideEffects = 0` is fine for reading CSRs, but not sure that there's some weird register out there that changes state after being read.
You are right. It is fairly possible and the previous version of this patch contained support for such general case (`Read_CSR` in https://reviews.llvm.org/D98936?vs=on&id=332569#toc). This pseudo is proposed for a particular case, which can be represented as a reading register. The concrete example is reading floating point control and state registers.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1176
+    PseudoInstExpansion<(CSRRW X0, SR.Encoding, GPR:$val)> {
+  let hasSideEffects = 0;
+  let Defs = Regs;
----------------
evandro wrote:
> This should definitely be `hasSideEffects = 1`.
Again, it is true in general case (see the removed pseudo `Write_CSR` in the previous version). This pseudo is for the special case, when write to system register has no effects other than changing content of the corresponding register. Floating point environment register behave exactly in this way.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1184
+    PseudoInstExpansion<(CSRRW GPR:$rd, SR.Encoding, GPR:$val)> {
+  let hasSideEffects = 0;
+  let Uses = Regs;
----------------
evandro wrote:
> As should this be `hasSideEffects = 1` too.
This is true for the removed `Swap_CSR` from the previous version. Maybe we need to put them back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98936