[RISCV] Implement c.lui immedate operand constraint

[shiva0217]

Summary

Implement c.lui immediate constraint to [1, 31] and [0xfffe0, 0xfffff].
[0xfffe0, 0xfffff] eventually will encode as [32, 63] as ISA describe.
The transformation from lui to c.lui could check the immediate range of c.lui directly.

[asb]

Thanks Shiva. I've left comments inline regarding the naming convention for this operand type, and the printing policy. As I say, I think we should let c.lui's operand print in the same way as lui for now. Consistently printing hex for lui and c.lui (as I think gnu objdump does) probably makes sense, but lets do that in a separate patch and do it for both instructions at the same time.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

276 ↗

(On Diff #132541)

Given that 'UImm6' is misleading I'd be tempted to just call this and the other related methods`isCLUIImm`. If preferring a isUImm naming scheme, isUImm20CLUI would actually be most consistent with the naming convention used for other asm operands, but that is also confusing. c.lui is a weird special case, so lets give it a name that reflects that.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

92 ↗

(On Diff #132541)

I don't think that the printing policy for c.lui should differ from lui. i.e. if we print it as '1048544' for lui then we should do the same for c.lui. We might later want to change the cases where hex is printed rather than decimal, but that would be better in a separate patch and applied consistently.

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

135 ↗

(On Diff #132541)

Drop unnecessary parens:

((Res >= 0xfffe0 && Res <= 0xfffff)) -> (Res >= 0xfffe0 && Res <= 0xfffff)

[shiva0217]

Update patch to reflect the comments.

[apazos]

lib/Target/RISCV/RISCVInstrInfoC.td

52 ↗

(On Diff #132783)

Thanks for the patch Shiva, I will launch a run to confirm the issue is resolved.

I am confused by the RISCV GCC comment, we are not doing this for compatibility with GCC, it is following the ISA right, though the ISA description for c.lui is not very clear. For example, c.srli better describes the accepted range. Shouldn't the ISA document be updated?

[apazos]

The run I did looks good, no more QEMU issue with illegal instr for C.LUI.

[asb]

Ana, can you share the encoded and decoded instruction which causes the qemu crash? With this patch it should be possible to generate, the same set of possible c.lui encodings as before (all legal), it's just the mapping from logical immediate operand -> encoded form that changes. i.e. before and after this patch, my expectation is it is _impossible_ to generate a c.lui that cases an illegal instruction assertion. Could well be a qemu bug of course.

I don't have time to check the git version of the spec, but it seems the error in 2.1 is listing c.lui immediate as nzuimm in the long-form definition rather than nzimm.

[shiva0217]

Hi Ana. The ISA probably not describe very straightforward. I update the comments try to illustrate more detail. Could you help me to check is the updated comments OK?

[apazos]

Hi Alex, I think you are right, the crashes were due to c.addi and c.addi16sp, which Shiva fixed in separate patch. I tested both patches together tough.

I think what broke was the assembler step when we were hand-modifying the test case to run on qemu to find out the offending instructions.

GCC assembler rejected:
echo 'c.lui x8, 32' |riscv32-unknown-linux-gnu-as -o sai -c -mabi=ilp32 -march=rv32gc
{standard input}: Assembler messages:
{standard input}:1: Error: illegal operands `c.lui x8,32'

while LLVM is accepting it:
echo 'c.lui x8, 32' |llvm-mc -triple=riscv32 -mattr=+c --show-encoding

.text
c.lui   s0, 32                  # encoding: [0x01,0x74]

which is fixed with this patch:

echo 'c.lui x8, 32' |llvm-mc -triple=riscv32 -mattr=+c --show-encoding

.text

<stdin>:1:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui x8, 32

[apazos]

LGTM.

lib/Target/RISCV/RISCVInstrInfoC.td

52 ↗

(On Diff #133401)

just some typo (checks, describes) and missing end point '.'.

[shiva0217]

Update the patch to reflect the comments.

[shiva0217]

Hi Alex. Do you think the patch is ready for landing?

[asb]

Only remaining concern:

RISCVMCCodeEmitter::getCLUIImmOpValue doesn't seem to be necessary - or at least, modifying c_lui_imm so it doesn't use a custom EncoderMethod still results in all tests passing. From my understanding of the code, I think the custom getCLUImmOpValue isn't necessary and can be removed. If I'm missing something and it is necessary, then the tests should be updated so they fail without it.

[shiva0217]

Hi Alex. You're right. RISCVGenMCCodeEmitter.inc generated by tablegen will create mask according to the bit-field width defined in *.td. Upper bit's will filter out by the mask, so we don't have to define custom encoding method for c.lui. Thanks!

[asb]

Looks good to me now. I had a go at rewording the c_lui_imm comment - feel free to take from that suggestion if you think it's any clearer.

lib/Target/RISCV/RISCVInstrInfoC.td

51–54 ↗

(On Diff #135396)

I had a go at rewording - does this seem any clearer to you? It's hard to explain concisely.

c_lui_imm checks the immediate range is in [1, 31] or [0xfffe0, 0xfffff]. The RISC-V ISA describes the constraint as [1, 63], with that value being loaded in to bits 17-12 of the destination register and sign extended from bit 17. Therefore, this 6-bit immediate can represent values in the ranges [1, 31] and [0xfffe0, 0xfffff].

llvm/trunk/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

bool isSImm6NonZero() const {
     if (!IsConstantImm)
       IsValid = RISCVAsmParser::classifySymbolRef(getImm(), VK, Imm);
     else
       IsValid = ((Imm != 0) && isInt<6>(Imm));
     return IsValid &&
            (VK == RISCVMCExpr::VK_RISCV_None || VK == RISCVMCExpr::VK_RISCV_LO);
   }
 
-  bool isUImm6NonZero() const {
+  bool isCLUIImm() const {
     int64_t Imm;
     RISCVMCExpr::VariantKind VK;
     bool IsConstantImm = evaluateConstantImm(Imm, VK);
-    return IsConstantImm && isUInt<6>(Imm) && (Imm != 0) &&
+    return IsConstantImm && (Imm != 0) &&
+           (isUInt<5>(Imm) || (Imm >= 0xfffe0 && Imm <= 0xfffff)) &&
             VK == RISCVMCExpr::VK_RISCV_None;
   }
 
   bool isUImm7Lsb00() const {
     int64_t Imm;
     RISCVMCExpr::VariantKind VK;
     bool IsConstantImm = evaluateConstantImm(Imm, VK);
     return IsConstantImm && isShiftedUInt<5, 2>(Imm) &&
            VK == RISCVMCExpr::VK_RISCV_None;
case Match_InvalidUImm5:
     return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 5) - 1);
   case Match_InvalidSImm6:
     return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 5),
                                       (1 << 5) - 1);
   case Match_InvalidSImm6NonZero:
     return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 5),
                                       (1 << 5) - 1,
         "immediate must be non-zero in the range");
-  case Match_InvalidUImm6NonZero:
-    return generateImmOutOfRangeError(Operands, ErrorInfo, 1, (1 << 6) - 1);
+  case Match_InvalidCLUIImm:
+    return generateImmOutOfRangeError(
+        Operands, ErrorInfo, 1, (1 << 5) - 1,
+        "immediate must be in [0xfffe0, 0xfffff] or");
   case Match_InvalidUImm7Lsb00:
     return generateImmOutOfRangeError(
         Operands, ErrorInfo, 0, (1 << 7) - 4,
         "immediate must be a multiple of 4 bytes in the range");
   case Match_InvalidUImm8Lsb00:
     return generateImmOutOfRangeError(
         Operands, ErrorInfo, 0, (1 << 8) - 4,
         "immediate must be a multiple of 4 bytes in the range");

llvm/trunk/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

static DecodeStatus decodeSImmOperandAndLsl1(MCInst &Inst, uint64_t Imm,
   assert(isUInt<N>(Imm) && "Invalid immediate");
   // Sign-extend the number in the bottom N bits of Imm after accounting for
   // the fact that the N bit immediate is stored in N-1 bits (the LSB is
   // always zero)
   Inst.addOperand(MCOperand::createImm(SignExtend64<N>(Imm << 1)));
   return MCDisassembler::Success;
 }
 
+static DecodeStatus decodeCLUIImmOperand(MCInst &Inst, uint64_t Imm,
+                                         int64_t Address,
+                                         const void *Decoder) {
+  assert(isUInt<6>(Imm) && "Invalid immediate");
+  if (Imm > 31) {
+    Imm = (SignExtend64<6>(Imm) & 0xfffff);
+  }
+  Inst.addOperand(MCOperand::createImm(Imm));
+  return MCDisassembler::Success;
+}
+
 #include "RISCVGenDisassemblerTables.inc"
 
 DecodeStatus RISCVDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
                                                ArrayRef<uint8_t> Bytes,
                                                uint64_t Address,
                                                raw_ostream &OS,
                                                raw_ostream &CS) const {
   // TODO: This will need modification when supporting instruction set

llvm/trunk/lib/Target/RISCV/RISCVInstrInfoC.td

 
 def simm6nonzero : Operand<XLenVT>,
                    ImmLeaf<XLenVT, [{return (Imm != 0) && isInt<6>(Imm);}]> {
   let ParserMatchClass = SImmAsmOperand<6, "NonZero">;
   let EncoderMethod = "getImmOpValue";
   let DecoderMethod = "decodeSImmOperand<6>";
 }
 
-def uimm6nonzero : Operand<XLenVT>,
-                   ImmLeaf<XLenVT, [{return isUInt<6>(Imm) && (Imm != 0);}]> {
-  let ParserMatchClass = UImmAsmOperand<6, "NonZero">;
-  let DecoderMethod = "decodeUImmOperand<6>";
+def CLUIImmAsmOperand : AsmOperandClass {
+  let Name = "CLUIImm";
+  let RenderMethod = "addImmOperands";
+  let DiagnosticType = !strconcat("Invalid", Name);
+}
+
+
+// c_lui_imm checks the immediate range is in [1, 31] or [0xfffe0, 0xfffff].
+// The RISC-V ISA describes the constraint as [1, 63], with that value being
+// loaded in to bits 17-12 of the destination register and sign extended from
+// bit 17. Therefore, this 6-bit immediate can represent values in the ranges
+// [1, 31] and [0xfffe0, 0xfffff].
+def c_lui_imm : Operand<XLenVT>,
+                ImmLeaf<XLenVT, [{return (Imm != 0) &&
+                                 (isUInt<5>(Imm) ||
+                                  (Imm >= 0xfffe0 && Imm <= 0xfffff));}]> {
+  let ParserMatchClass = CLUIImmAsmOperand;
+  let EncoderMethod = "getImmOpValue";
+  let DecoderMethod = "decodeCLUIImmOperand";
 }
 
 // A 7-bit unsigned immediate where the least significant two bits are zero.
 def uimm7_lsb00 : Operand<XLenVT>,
                   ImmLeaf<XLenVT, [{return isShiftedUInt<5, 2>(Imm);}]> {
   let ParserMatchClass = UImmAsmOperand<7, "Lsb00">;
   let EncoderMethod = "getImmOpValue";
   let DecoderMethod = "decodeUImmOperand<7>";
def C_ADDI16SP : RVInst16CI<0b011, 0b01, (outs SP:$rd_wb),
   let Inst{6} = imm{4};
   let Inst{5} = imm{6};
   let Inst{4-3} = imm{8-7};
   let Inst{2} = imm{5};
 }
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_LUI : RVInst16CI<0b011, 0b01, (outs GPRNoX0X2:$rd),
-                       (ins uimm6nonzero:$imm),
+                       (ins c_lui_imm:$imm),
                        "c.lui", "$rd, $imm"> {
   let Inst{6-2} = imm{4-0};
 }
 
 def C_SRLI : Shift_right<0b00, "c.srli", GPRC, uimmlog2xlennonzero>;
 def C_SRAI : Shift_right<0b01, "c.srai", GPRC, uimmlog2xlennonzero>;
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in

llvm/trunk/test/MC/RISCV/rv32c-invalid.s

 c.li t0, 128 # CHECK: :[[@LINE]]:10: error: immediate must be an integer in the range [-32, 31]
 c.andi a0, -33 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [-32, 31]
 
 ## simm6nonzero
 c.addi t0, 0 # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
 c.addi t0, -33 # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
 c.addi t0, 32 # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
 
-## uimm6nonzero
-c.lui t0, 64 # CHECK: :[[@LINE]]:11: error: immediate must be an integer in the range [1, 63]
-c.lui t0, 0 # CHECK: :[[@LINE]]:11: error: immediate must be an integer in the range [1, 63]
+## c_lui_imm
+c.lui t0, 0 # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
+c.lui t0, 32 # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
+c.lui t0, 0xffffdf # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
+c.lui t0, 0x1000000 # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
 
 ## uimm8_lsb00
 c.lwsp  ra, 256(sp) # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 4 bytes in the range [0, 252]
 c.swsp  ra, -4(sp) # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 4 bytes in the range [0, 252]
 ## uimm7_lsb00
 c.lw  s0, -4(sp) # CHECK: :[[@LINE]]:11: error: immediate must be a multiple of 4 bytes in the range [0, 124]
 c.sw  s0, 128(sp) # CHECK: :[[@LINE]]:11: error: immediate must be a multiple of 4 bytes in the range [0, 124]
 

llvm/trunk/test/MC/RISCV/rv32c-valid.s

 # CHECK: encoding: [0x01,0x00]
 c.nop
 # CHECK-INST: c.ebreak
 # CHECK: encoding: [0x02,0x90]
 c.ebreak
 # CHECK-INST: c.lui   s0, 1
 # CHECK: encoding: [0x05,0x64]
 c.lui   s0, 1
-# CHECK-INST: c.lui   s0, 63
+# CHECK-INST: c.lui   s0, 31
+# CHECK: encoding: [0x7d,0x64]
+c.lui   s0, 31
+# CHECK-INST: c.lui   s0, 1048544
+# CHECK: encoding: [0x01,0x74]
+c.lui   s0, 0xfffe0
+# CHECK-INST: c.lui   s0, 1048575
 # CHECK: encoding: [0x7d,0x74]
-c.lui   s0, 63
+c.lui   s0, 0xfffff