riscv-software-src / riscv-isa-sim

Spike, a RISC-V ISA Simulator
Other
2.28k stars 813 forks source link

Legal ISA string option not accepted #1696

Closed christian-herber-nxp closed 2 weeks ago

christian-herber-nxp commented 2 weeks ago

These strings are accepted:

spike  --isa=rv32im_zce_zilsd_zcmlsd
spike  --isa=rv32im_za_zilsd_zcmlsd

While this isn't:

spike  --isa=rv32imc_zilsd_zcmlsd_zfinx
error: bad --isa option 'rv32imc_zilsd_zcmlsd_zfinx'. 'Zcmlsd' extension requires 'Zca' and 'Zilsd' extensions

From the isa_parser, I would expect that the second one should also work, as having C should assert Zca:

  if (extension_table['C']) {
    extension_table[EXT_ZCA] = true;
    if (extension_table['F'] && max_xlen == 32)
      extension_table[EXT_ZCF] = true;
    if (extension_table['D'])
      extension_table[EXT_ZCD] = true;
  }

It seems that extension_table['C'] is not set anywhere in the isa_parser.

aswaterman commented 2 weeks ago

This does appear to be a bug. Would you like to take a stab at this one, or should one of us handle it later this week?

christian-herber-nxp commented 2 weeks ago

I couldn't find the code that is supposed to set extension_table['C'] == true, so honestly, I have no idea where to look to fix this.

aswaterman commented 2 weeks ago

OK, I'll take this one.

aswaterman commented 2 weeks ago

For future reference, this is where extension_table['C'] is set: https://github.com/riscv-software-src/riscv-isa-sim/blob/5e7928a3475f3fe6a148abd74abb6d97c65c31f2/disasm/isa_parser.cc#L78

So the problem is just that the logic that handles C -> Zca occurs too late in the procedure; it needs to be hoisted up before the error-checking code.