[RISCV] For rv32, accept constants like 0xfffff800 as a valid simm12.

[craig.topper]

Summary

Internally we store constants in int64_t after parsing, but this is
kind of an implementation detail. If we only supported rv32, we might
have chosen int32_t.

For rv32, I think it makes sense to accept the constants that we
would accept if int32_t was the internal type. In fact we already
do this for the li alias. This patch extends this to sign
extended constants for other instructions.

This matches the GNU assembler. The difference between LLVM and gcc
was previously noted here. https://github.com/riscv-non-isa/riscv-asm-manual/pull/71

[luismarques]

The reasoning and approach makes sense to me. I haven't (yet) checked the implementation and test coverage in detail, so this is a weak approval only.

llvm/test/MC/RISCV/rv32i-only-valid.s

10

Please add a comment with the hex representation. Personally, I can't eyeball these large ones and be sure it's exactly 0xFFFFFFFF.

[craig.topper]

Rename function.
Add comment to test.

[reames]

Basic idea makes complete sense to me. One question on implementation.

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

772

Is this checking the right thing? It seems like maybe we want to allow the 32 bit form on the Imm -1, not the Imm. Main case I'm thinking of is what if imm is smin<5> and thus smin<5>-1 is not an int<5>.

Alternatively said, please check what gcc does and make sure our behavior matches in the edge case.

[craig.topper]

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

772

I think it's right. Imm is the value written by the user. If I sign extend after subtracting 1 we will accept 0x100000000 the same as if the user had written 0. Because 0x100000000 will be decremented to 0xffffffff which will sign extend to 0xffffffffffffffff. We also get 0xffffffffffffffff from subtracting 1 from 0.

The valid input values should be the ranges [-15, 16] and [0xfffffff1, 0xffffffff].

[reames]

LGTM

[luismarques]

I suppose we could also have added S-type instructions tests, for thoroughness.

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

 //===-- RISCVAsmParser.cpp - Parse RISCV assembly to MCInst instructions --===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
public:
   }
 
   bool isSImm5() const {
     if (!isImm())
       return false;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     int64_t Imm;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
-    return IsConstantImm && isInt<5>(Imm) && VK == RISCVMCExpr::VK_RISCV_None;
+    return IsConstantImm && isInt<5>(fixImmediateForRV32(Imm, isRV64Imm())) &&
+           VK == RISCVMCExpr::VK_RISCV_None;
   }
 
   bool isSImm6() const {
     if (!isImm())
       return false;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     int64_t Imm;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
-    return IsConstantImm && isInt<6>(Imm) && VK == RISCVMCExpr::VK_RISCV_None;
+    return IsConstantImm && isInt<6>(fixImmediateForRV32(Imm, isRV64Imm())) &&
+           VK == RISCVMCExpr::VK_RISCV_None;
   }
 
   bool isSImm6NonZero() const {
     if (!isImm())
       return false;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     int64_t Imm;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
-    return IsConstantImm && isInt<6>(Imm) && (Imm != 0) &&
+    return IsConstantImm && Imm != 0 &&
+           isInt<6>(fixImmediateForRV32(Imm, isRV64Imm())) &&
            VK == RISCVMCExpr::VK_RISCV_None;
   }
 
   bool isCLUIImm() const {
     if (!isImm())
       return false;
     int64_t Imm;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
if (!isImm())
       return false;
     int64_t Imm;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
     return IsConstantImm && isShiftedUInt<8, 2>(Imm) && (Imm != 0) &&
            VK == RISCVMCExpr::VK_RISCV_None;
   }
 
+  // If this a RV32 and the immediate is a uimm32, sign extend it to 32 bits.
+  // This allows writing 'addi a0, a0, 0xffffffff'.
+  static int64_t fixImmediateForRV32(int64_t Imm, bool IsRV64Imm) {
+    if (IsRV64Imm || !isUInt<32>(Imm))
+      return Imm;
+    return SignExtend64<32>(Imm);
+  }
+
   bool isSImm12() const {
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     int64_t Imm;
     bool IsValid;
     if (!isImm())
       return false;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
     if (!IsConstantImm)
       IsValid = RISCVAsmParser::classifySymbolRef(getImm(), VK);
     else
-      IsValid = isInt<12>(Imm);
+      IsValid = isInt<12>(fixImmediateForRV32(Imm, isRV64Imm()));
     return IsValid && ((IsConstantImm && VK == RISCVMCExpr::VK_RISCV_None) ||
                        VK == RISCVMCExpr::VK_RISCV_LO ||
                        VK == RISCVMCExpr::VK_RISCV_PCREL_LO ||
                        VK == RISCVMCExpr::VK_RISCV_TPREL_LO);
   }
 
   bool isSImm12Lsb0() const { return isBareSimmNLsb0<12>(); }
 
public:
   }
 
   bool isSImm5Plus1() const {
     if (!isImm())
       return false;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     int64_t Imm;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
-    return IsConstantImm && isInt<5>(Imm - 1) &&
+    return IsConstantImm &&
+           isInt<5>(fixImmediateForRV32(Imm, isRV64Imm()) - 1) &&
            VK == RISCVMCExpr::VK_RISCV_None;
   }
 
   /// getStartLoc - Gets location of the first token of this operand
   SMLoc getStartLoc() const override { return StartLoc; }
   /// getEndLoc - Gets location of the last token of this operand
   SMLoc getEndLoc() const override { return EndLoc; }
   /// True if this operand is for an RV64 instruction
public:
   static std::unique_ptr<RISCVOperand> createVType(unsigned VTypeI, SMLoc S) {
     auto Op = std::make_unique<RISCVOperand>(KindTy::VType);
     Op->VType.Val = VTypeI;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static void addExpr(MCInst &Inst, const MCExpr *Expr) {
+  static void addExpr(MCInst &Inst, const MCExpr *Expr, bool IsRV64Imm) {
     assert(Expr && "Expr shouldn't be null!");
     int64_t Imm = 0;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     bool IsConstant = evaluateConstantImm(Expr, Imm, VK);
 
     if (IsConstant)
-      Inst.addOperand(MCOperand::createImm(Imm));
+      Inst.addOperand(
+          MCOperand::createImm(fixImmediateForRV32(Imm, IsRV64Imm)));
     else
       Inst.addOperand(MCOperand::createExpr(Expr));
   }
 
   // Used by the TableGen Code
   void addRegOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::createReg(getReg()));
   }
 
   void addImmOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
-    addExpr(Inst, getImm());
+    addExpr(Inst, getImm(), isRV64Imm());
   }
 
   void addFenceArgOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::createImm(Fence.Val));
   }
 
   void addCSRSystemRegisterOperands(MCInst &Inst, unsigned N) const {

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

 
 # CHECK-OBJ: c.jal 0x7fe
 # CHECK-ASM: c.jal 2046
 # CHECK-ASM: encoding: [0xfd,0x2f]
 # CHECK-NO-EXT: error: instruction requires the following: 'C' (Compressed Instructions) or 'Zca' (part of the C extension, excluding compressed floating point loads/stores){{$}}
 # CHECK-NO-RV32: error: instruction requires the following: RV32I Base Instruction Set{{$}}
 # CHECK-NO-RV32-AND-EXT: error: instruction requires the following: 'C' (Compressed Instructions) or 'Zca' (part of the C extension, excluding compressed floating point loads/stores), RV32I Base Instruction Set{{$}}
 c.jal 2046
+
+# CHECK-OBJ: c.addi a1, -1
+# CHECK-ASM: c.addi a1, -1
+# CHECK-ASM: encoding: [0xfd,0x15]
+c.addi a1, 0xffffffff

llvm/test/MC/RISCV/rv32i-only-valid.s

+# RUN: llvm-mc %s -triple=riscv32 -riscv-no-aliases -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 < %s \
+# RUN:     | llvm-objdump -M no-aliases -d -r - \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: addi a0, a1, -1
+# CHECK-ASM: encoding: [0x13,0x85,0xf5,0xff]
+addi a0, a1, 4294967295 # 0xffffffff
+# CHECK-ASM-AND-OBJ: ori a2, a3, -2048
+# CHECK-ASM: encoding: [0x13,0xe6,0x06,0x80]
+ori a2, a3, 0xfffff800

llvm/test/MC/RISCV/rvv/rv32-immediate.s

+# RUN: llvm-mc -triple=riscv32 -show-encoding --mattr=+v %s \
+# RUN:        | FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST
+# RUN: llvm-mc -triple=riscv32 -filetype=obj --mattr=+v %s \
+# RUN:        | llvm-objdump -d --mattr=+v - \
+# RUN:        | FileCheck %s --check-prefix=CHECK-INST
+
+# For rv32, allow 32 bit constants that contains a simm5 value.
+
+vadd.vi v8, v4, 0xfffffff0
+# CHECK-INST: vadd.vi v8, v4, -16
+# CHECK-ENCODING: [0x57,0x34,0x48,0x02]
+
+vmsltu.vi v8, v4, 0xfffffff1
+# CHECK-INST: vmsleu.vi v8, v4, -16
+# CHECK-ENCODING: [0x57,0x34,0x48,0x72]