[RISCV] Use RVInst16CB for C_SRLI64_HINT and C_SRAI64_HINT.

[topperc]

c.srli(64) and c.srai(64) are encoded differently than c.slli(64). The former have a 3-bit register, while the latter has a 5-bit register. c.srli and c.srai use RVInst16CB.

The "let Inst{11-10} =" prevented this from causing any functional issues by dropping the upper 2 bits of the register. The ins/outs list uses GPRC so the register class is constrained.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

c.srli(64) and c.srai(64) are encoded differently than c.slli(64). The former have a 3-bit register, while the latter has a 5-bit register. c.srli and c.srai use RVInst16CB.

The "let Inst{11-10} =" prevented this from causing any functional issues by dropping the upper 2 bits of the register. The ins/outs list uses GPRC so the register class is constrained.

---

Full diff: https://github.com/llvm/llvm-project/pull/112250.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVInstrInfoC.td (+10-10)

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
index 7d742322b42969..c9f880f1565192 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
@@ -705,23 +705,23 @@ def C_SLLI64_HINT : RVInst16CI<0b000, 0b10, (outs GPR:$rd_wb), (ins GPR:$rd),
   let Inst{12} = 0;
 }
 
-def C_SRLI64_HINT : RVInst16CI<0b100, 0b01, (outs GPRC:$rd_wb),
-                               (ins GPRC:$rd),
-                               "c.srli64", "$rd">,
+def C_SRLI64_HINT : RVInst16CB<0b100, 0b01, (outs GPRC:$rs1_wb),
+                               (ins GPRC:$rs1),
+                               "c.srli64", "$rs1">,
                     Sched<[WriteShiftImm, ReadShiftImm]> {
-  let Constraints = "$rd = $rd_wb";
+  let Constraints = "$rs1 = $rs1_wb";
   let Inst{6-2} = 0;
-  let Inst{11-10} = 0;
+  let Inst{11-10} = 0b00;
   let Inst{12} = 0;
 }
 
-def C_SRAI64_HINT : RVInst16CI<0b100, 0b01, (outs GPRC:$rd_wb),
-                               (ins GPRC:$rd),
-                               "c.srai64", "$rd">,
+def C_SRAI64_HINT : RVInst16CB<0b100, 0b01, (outs GPRC:$rs1_wb),
+                               (ins GPRC:$rs1),
+                               "c.srai64", "$rs1">,
                     Sched<[WriteShiftImm, ReadShiftImm]> {
-  let Constraints = "$rd = $rd_wb";
+  let Constraints = "$rs1 = $rs1_wb";
   let Inst{6-2} = 0;
-  let Inst{11-10} = 1;
+  let Inst{11-10} = 0b01;
   let Inst{12} = 0;
 }
 

[lenary]

LGTM

[lenary]

I was recently looking at how the backend chooses to do an RVC encoding vs a regular encoding, and was fairly surprised that it's effectively implicitly done from the tablegen insts - rather than us having a separate table. I'm not sure how we would do a separate table (as it's not a HwMode thing, it's a per-instruction thing), but this is the kind of thing I think that cannot be caught with the implicit approach (even though there's no actual bug here).

[topperc]

I was recently looking at how the backend chooses to do an RVC encoding vs a regular encoding, and was fairly surprised that it's effectively implicitly done from the tablegen insts - rather than us having a separate table. I'm not sure how we would do a separate table (as it's not a HwMode thing, it's a per-instruction thing), but this is the kind of thing I think that cannot be caught with the implicit approach (even though there's no actual bug here).

I'm not sure I follow. The compression is done via the CompressPat class in Tablegen which links the compressed and uncompressed instruction names. That generates a MachineInstr compressor for size estimation, and a MCInst compressor for RISCVAsmPrinter and the assembler. Then one decompressor for the disassembler.

[lenary]

I'm not talking about the CompressPat, I'm talking about once you have a C_ANDI, and you ask for its encoding using getBinaryCodeForInstr, that calls getMachineOpValue for the rd operand (the RVC-encoded register). getMachineOpValue returns the 5-bit encoding, which then the generated code in getBinaryCodeForInstr masks down to 3 bits to put the right bits in the instruction encoding. In that code there is no assert that we actually have an RVC register, rather than another register, though maybe that's not the right place in the backend to check that - I don't know where that code would be.

But I find it interesting that we don't have something like a tablegen instance of RegisterOperand with a custom EncoderMethod, and instead just trust the extraction of 3 bits from the 5 provided by getMachineOpValue.

To be clear, I've not found a bug, I just have an instinct not to trust this kind of implicit behaviour.

[topperc]

I'm not talking about the CompressPat, I'm talking about once you have a C_ANDI, and you ask for its encoding using getBinaryCodeForInstr, that calls getMachineOpValue for the rd operand (the RVC-encoded register). getMachineOpValue returns the 5-bit encoding, which then the generated code in getBinaryCodeForInstr masks down to 3 bits to put the right bits in the instruction encoding. In that code there is no assert that we actually have an RVC register, rather than another register, though maybe that's not the right place in the backend to check that - I don't know where that code would be.

But I find it interesting that we don't have something like a tablegen instance of RegisterOperand with a custom EncoderMethod, and instead just trust the extraction of 3 bits from the 5 provided by getMachineOpValue.

To be clear, I've not found a bug, I just have an instinct not to trust this kind of implicit behaviour.

Thanks for the clarification. I thought you meant the full compressed instruction encoding, but now I see you meant the register encoding.