Substitued XLEN with MXLEN #80
Conversation
Signed-off-by: Afonso Oliveira <[email protected]>
There was a problem hiding this comment.
This was super helpful to see. Now let's back it out 😃
Most of the references to xlen should stay xlen since they aren't tied to a specific priv mode. I started to comment on each one, but it's probably easier to revert and then add in what's needed:
Looking through your changes, the only renames of lowercase xlen -> mxlen I see that should stay are in:
- MC-1.yaml
- type.rb
- csc_crd.rb
There are also several instances of an uppercase XLEN that should change but I don't see here. For example, in arch_def.rb:
@mxlen = @arch_def.dig("params", "XLEN") -> should be "MXLEN"
Similarly in arch_gen.rb, csc.rb, ast.rb, symbol_table.rb, and helpers.rb ("XLEN" needs to become "MXLEN").
Note that after you change ast.rb, then the IDL code will need changed, too. The grammar in idl.treetop will need changed (XLEN->MXLEN everywhere) and any integer literal specifying XLEN as a bit width will need changed (e.g., XLEN'h3f -> MXLEN'h3f in globals.isa).
| @@ -23,7 +23,7 @@ mcycle: | |||
| sw_write(csr_value): | | |||
| # since writes to this register may not be hart-local, it must be handled | |||
| # as a special case | |||
| if (xlen() == 32) { | |||
There was a problem hiding this comment.
xlen() should stay as is; it is supposed to return the effective XLEN in the current mode (i.e., could be MXLEN, SXLEN, UXLEN, ...)
(Note: There are a bunch of these)
| @@ -1167,18 +1167,18 @@ function base32? { | |||
| return True iff current effective XLEN == 32 | |||
| } | |||
| body { | |||
| XRegWidth xlen32 = XRegWidth::XLEN32; | |||
| XRegWidth mxlen32 = XRegWidth::XLEN32; | |||
There was a problem hiding this comment.
Functionally, this is fine. But here 'xlen32' is just supposed to be an alias of XRegWidth::XLEN32 so it doesn't have be written out every time in the rest of the function.
| @@ -100,7 +100,7 @@ This field has special behavior when written by software (_e.g._, through `csrrw | |||
| + | |||
| When software tries to write `csr_value`, the field will be written with the return value of the function below. | |||
| + | |||
| <%- if arch_def.multi_xlen? && csr.defined_in_all_bases? && field.defined_in_all_bases? -%> | |||
| <%- if arch_def.multi_mxlen? && csr.defined_in_all_bases? && field.defined_in_all_bases? -%> | |||
There was a problem hiding this comment.
multi_xlen should stay multi_xlen; it is indicating whether or not, in some privilege mode, the xlen can change between 32 and 64 (not M-mode specific)
|
|
||
| <%- if inst.key?("operation()") -%> | ||
| [tabs] | ||
| ==== | ||
| <%- xlens.each do |effective_xlen| -%> | ||
| Pruned, XLEN == <%= effective_xlen %>:: | ||
| <%- xlens.each do |effective_mxlen| -%> |
There was a problem hiding this comment.
Did you mean to check in this update to riscv-opcodes?
There was a problem hiding this comment.
No. I believe it should be updated if we're using it but it should not have been commited here. I'll also remove it and add it when we start moving instructions from auto-instr.
| @@ -49,7 +49,7 @@ def defined_in_all_bases? = @data["base"].nil? | |||
|
|
|||
| # @param arch_def [ArchDef] A configuration | |||
| # @return [Boolean] Whether or not the format of this CSR changes when the effective XLEN changes in some mode | |||
| def format_changes_with_xlen?(arch_def) | |||
| def format_changes_with_mxlen?(arch_def) | |||
There was a problem hiding this comment.
This should stay format_changes_with_xlen; it's not M-mode specific
|
Thank you so much for the careful review! I'll try to address them all and get back to you in the next days in order to properly fix the PR. |
|
Closing for now; we are going to circle back to this once we have the "offical" list of parameters in hand. |
Closes #52 . I've built the manual with the updates and nothing was broken.