[PATCH] D136847: [RISCV][NFC] Mark rs1 in most memory instructions as memory operand.

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:131
 
 def GPRMemZeroOffset : RegisterOperand<GPR> {
+  let OperandType = "OPERAND_MEMORY";
----------------
Or change the RegisterOperand<GPR> to MemOperand<GPR>


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:137-143
+class MemOperand<RegisterClass regClass>: RegisterOperand<regClass>{
+  let OperandType = "OPERAND_MEMORY";
+}
+
+def GPRMemOperand: MemOperand<GPR>;
+def SPMemOperand: MemOperand<SP>;
+def GPRCMemOperand: MemOperand<GPRC>;
----------------
You're missing a bunch of whitespace before tokens


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:141
+
+def GPRMemOperand: MemOperand<GPR>;
+def SPMemOperand: MemOperand<SP>;
----------------
I don't think we want an Operand suffix on these, we don't for other things


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:505
 class Load_ri<bits<3> funct3, string opcodestr>
-    : RVInstI<funct3, OPC_LOAD, (outs GPR:$rd), (ins GPR:$rs1, simm12:$imm12),
+    : RVInstI<funct3, OPC_LOAD, (outs GPR:$rd), (ins GPRMemOperand:$rs1, simm12:$imm12),
               opcodestr, "$rd, ${imm12}(${rs1})">;
----------------
So what about all the immediate offsets? OPERAND_MEMORY surely isn't very useful without knowing the offset? It just tells you the instruction is touching memory somewhere in the +/- 2 KiB region centred around that register.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136847