Fix issue #258

[freshLiver]

Description

As the issue desciption said, in the latest Ripes, the I-type instructions will check whether the given immediate part can fit in the instruction limitation.

For example, the addi instruction will ensure the given immediate is less than 13 bits, as the following image shows:

However, the lui instruction's immediate part should only accept an immediate value that could fit in 20 bits, but current version doesn't handle this limitation correctly, as shown in the above image.

Tracing

To find the problem, I use std::cout to dump the width in the checkFitsInWidth function, which is for checking the immediate range:

static Result<> checkFitsInWidth(Reg_T_S value, const Location &sourceLine,
                                  ImmConvInfo &convInfo,
                                  QString token = QString()) {

  std::cout << __PRETTY_FUNCTION__ << std::endl
            << "check token '" << token.toStdString() << "' (expected width=" << width
            << ") at #" << sourceLine.sourceLine() << std::endl;

  ...

Then, rebuild the Ripes and test it with the following codes:

main:
    addi x1, x1, 0x123455
    lui x1, 0x12345678

And found that the width of lui instruction is misconfigured as 32, instead of 20:

static Ripes::Result<> Ripes::ImmBase<tokenIndex, width, repr, ImmParts, symbolType, transformer>::checkFitsInWidth(Ripes::ImmBase<tokenIndex, width, repr, ImmParts, symbolType, transformer>::Reg_T_S, const Ripes::Location&, Ripes::ImmConvInfo&, QString) [with unsigned int tokenIndex = 2; unsigned int width = 12; Ripes::Repr repr = Ripes::Repr::Signed; ImmParts = Ripes::ImmPartBase<0, Ripes::BitRange<20, 31, 32> >; Ripes::SymbolType symbolType = Ripes::SymbolType::None; Ripes::Reg_T (* transformer)(Ripes::Reg_T) = Ripes::defaultTransformer; Ripes::ImmBase<tokenIndex, width, repr, ImmParts, symbolType, transformer>::Reg_T_S = long int]
check token '0x123455' (expected width=12) at #1
static Ripes::Result<> Ripes::ImmBase<tokenIndex, width, repr, ImmParts, symbolType, transformer>::checkFitsInWidth(Ripes::ImmBase<tokenIndex, width, repr, ImmParts, symbolType, transformer>::Reg_T_S, const Ripes::Location&, Ripes::ImmConvInfo&, QString) [with unsigned int tokenIndex = 1; unsigned int width = 32; Ripes::Repr repr = Ripes::Repr::Hex; ImmParts = Ripes::ImmPartBase<0, Ripes::BitRange<12, 31, 32> >; Ripes::SymbolType symbolType = Ripes::SymbolType::None; Ripes::Reg_T (* transformer)(Ripes::Reg_T) = Ripes::defaultTransformer; Ripes::ImmBase<tokenIndex, width, repr, ImmParts, symbolType, transformer>::Reg_T_S = long int]
check token '0x12345678' (expected width=32) at #2

And this is origin from the the U-type instruction implementation, we can see that the width is configured as 32:

/// A RISC-V immediate field with an input width of 32 bits.
/// Used in U-Type instructions.
///
/// It is defined as:
///  - Imm[31:12] = Inst[31:12]
///  - Imm[11:0]  = 0
constexpr static unsigned VALID_INDEX = 1;
template <unsigned index, SymbolType symbolType>
struct ImmU
    : public ImmSym<index, 32, Repr::Hex, ImmPart<0, 12, 31>, symbolType> {
  static_assert(index == VALID_INDEX, "Invalid token index");
};

/// A U-Type RISC-V instruction
template <typename InstrImpl, RVISA::OpcodeID opcodeID,
          SymbolType symbolType = SymbolType::None>
class Instr : public RV_Instruction<InstrImpl> {
  template <unsigned index>
  using Imm = ImmU<index, symbolType>;

public:
  struct Opcode : public OpcodeSet<OpPartOpcode<opcodeID>> {};
  struct Fields : public FieldSet<RegRd, Imm> {};
};

struct Auipc
    : public Instr<Auipc, RVISA::OpcodeID::AUIPC, SymbolType::Absolute> {
  constexpr static std::string_view NAME = "auipc";
};

struct Lui : public Instr<Lui, RVISA::OpcodeID::LUI> {
  constexpr static std::string_view NAME = "lui";
};

} // namespace TypeU

Solve

By changing the width to 20, the bit range checking now works as expected, on the lui and auipc :

Testing

Currently I only use the provided testing utility for checking my changes didn't break it:

/ripes/build/test$ ./tst_assembler && ./tst_expreval && ./tst_riscv

...

Totals: 10 passed, 0 failed, 0 skipped, 0 blacklisted, 9676ms
********* Finished testing of tst_RISCV *********

However, since I'm not that familiar with C++, I'm not sure whether I understood the bit range checking process correctly. If there is any problem in my changes, please me know.

[mortbopet]

Thank you for submitting a fix! To make sure that we test this, i'd suggest adding a test in the same style and file as this where you create test cases for Expect::Fail and Expect::Success that excercises this change.

[freshLiver]

Thank you for the suggestion! I have added some testing for checking immediate bit range, but currently only for RV32I instructions since I'm not familiar with the [CM]-extensions and the 64-bits instructions.

[freshLiver]

While adding the remaining testing cases, I encountered similar situations again on other instructions.

In these cases, the issue arises from directly setting the width to the actual width for executing the instruction, rather than configuring it based on the allowed number of imm bits specified in the instruction. Therefore, instructions that involve padding zeros seem to share the problem.

For example, in the case of beq instruction, even though it allows the branch range to be ±4K (13 bits), due to imm[0] always being 0 to ensure the offset is a multiple of 2, effectively only 12 bits are used for imm.

/// A RISC-V signed immediate field with an input width of 13 bits.
/// Used in B-Type instructions.
///
/// It is defined as:
///  - Imm[31:12] = Inst[31]
///  - Imm[11]    = Inst[7]
///  - Imm[10:5]  = Inst[30:25]
///  - Imm[4:1]   = Inst[11:8]
///  - Imm[0]     = 0
constexpr static unsigned VALID_INDEX = 2;
template <unsigned index>
struct ImmB : public ImmSym<index, 13, Repr::Signed,
                            ImmPartSet<ImmPart<12, 31, 31>, ImmPart<11, 7, 7>,
                                       ImmPart<5, 25, 30>, ImmPart<1, 8, 11>>,
                            SymbolType::Relative> {
  static_assert(index == VALID_INDEX, "Invalid token index");
};

As these scenarios, which appear to be errors, are somewhat related to the instruction processing rather than simple typos, I would like to confirm whether these should be classified as errors and could directly modify the declared width when defining instructions without affecting the original instruction processing.