ISA extension spec probably should not require intrinsics

[Timmmm]

Currently there's a global register:

/* internal state to hold instruction bits for faulting instructions */
register instbits : xlenbits

Which is saved after fetch:

                F_Base(w) => {
                  instbits = zero_extend(w);

And used in handle_illegal():

function handle_illegal() -> unit = {
  let info = if plat_mtval_has_illegal_inst_bits ()
             then Some(instbits)
             else None();

This use of a global is unnecessary. It would be more elegant to:

1. Remove the instbits register.
2. Add RETIRE_ILLEGAL to the Retire enum.
3. Change all instances of handle_illegal(); RETIRE_FAIL to just RETIRE_ILLEGAL
4. Add instbits as a parameter to handle_illegal().
5. Call handle_illegal(...) from step() if retired == RETIRE_ILLEGAL.

[jrtc27]

Whilst I agree that the global is ugly, I disagree that your proposal is a better fit for the architectural specification. Architecturally, illegal instruction is just another exception, and so should follow the exact same path as other exceptions.

Which I think means what you really want is that all exception cases are RETIRE_FAIL(exception_details), with the top level then being responsible for the exception handling, passing on the extra state needed, including instbits for illegal instructions.

[Timmmm]

Good call, that does sound better and I guess would let you treat other exceptions in the same way (I'll have to double check the code though). I don't think exception_details needs to include the instructions bits though since they are already available at the point in step() where it's handled (and they'd be a bit awkward to get in execute() since the instruction has already been decoded).

[jrtc27]

Good call, that does sound better and I guess would let you treat other exceptions in the same way (I'll have to double check the code though). I don't think exception_details needs to include the instructions bits though since they are already available at the point in step() where it's handled (and they'd be a bit awkward to get in execute() since the instruction has already been decoded).

Sorry I should have been clearer: instbits would be supplied to handle_exception to use in the case that the execute clause returned Exc_Illegal_Inst (names entirely made up, any semblance to real names is coincidental). Or, to put it another way: yes, that's what I meant, I just failed to clearly communicate it.

[martinberger]

I'm also in favour of extending the Retired data type.

There is another important case: Sdtrig debug triggers (https://github.com/riscv/riscv-debug-spec). One way to implement is to centralise trigger handling uniformly in step. But if you get a trigger for example of a load address, that trigger is found in the execute clause of the relevant load instruction. We don't want to handle the trigger in all relevant execute clauses, so need to pass this information back to step where it can be handled uniformly. (And where we need to handle triggers on the PC anyway.) For this purpose a RETIRE_TRIGGER(...) would be neater than yet another set of global, but non architectural registers.

Extending Retired does not require changing all the existing code that returns RETIRE_SUCCESS or RETIRE_FAIL, so it's not an intrusive change.