Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fence.tso is not pseudo for Fence #428

Open
AFOliveira opened this issue Jan 20, 2025 · 6 comments · May be fixed by #460
Open

Fence.tso is not pseudo for Fence #428

AFOliveira opened this issue Jan 20, 2025 · 6 comments · May be fixed by #460

Comments

@AFOliveira
Copy link
Collaborator

Currently UDB specified fence.tso as pseudo-instructions under fence.yaml. However, this is not the case and fence.tso should have a separate file for it.

I believe this is the specified like this because of riscv-opcodes, but the only reason riscv-opcodes specified it like that is it not possible to support for both on current riscv-opcodes format.

Context:
riscv/riscv-opcodes#330
riscv/riscv-isa-manual#1782 (comment)

@AFOliveira AFOliveira added data error An error in the database data and removed data error An error in the database data labels Jan 20, 2025
@ThinkOpenly
Copy link
Collaborator

fence.tso should have a separate file for it.

Probably so. It'll make a bit of a mess because you'll need to exclude the specific FENCE.TSO encoding from the FENCE encoding.

@dhower-qc
Copy link
Collaborator

Agree. In terms of encoding, this is captured by the hints key (I might not have that in main yet; it's used by the ISS decoder). Maybe there is a better key name?

fence.yaml:

# ...
hints:
  - { $ref: "inst/I/fence.tso.yaml" }
# ...

fence.tso.yaml:

# full fence.tso instruction

@AFOliveira
Copy link
Collaborator Author

AFOliveira commented Jan 22, 2025

I can't comprehend why to encode fence.tso as hint. Do they need to be connected at all? I think FENCE is described in the ISA both as a category of intructions (everything related to FENCEs) and as just an instruction FENCE.

Image

I think it kind of acts the same way as the funct3/funct7 fields for Register-Register operations

Image

Can't we remove the fm field from fence as a variable field and just describe it as 0000 for FENCE and 0001 for FENCE.TSO or would that have any further implications?

@ThinkOpenly
Copy link
Collaborator

How do we represent "reserved" values for which there is a "shall" in the ISA spec:

Likewise, many _fm_ [...] settings [...] are also reserved for future use. Base implementations shall treat all such reserved configurations as normal fences with fm=0000

Can't we remove the fm field from fence as a variable field and just describe it as 0000 for FENCE and 0001 for FENCE.TSO or would that have any further implications?

That's actually what the Sail code does:

mapping clause encdec = FENCE(pred, succ)
  <-> 0b0000 @ pred @ succ @ 0b00000 @ 0b000 @ 0b00000 @ 0b0001111
[...]
mapping clause encdec = FENCE_TSO(pred, succ)
  <-> 0b1000 @ pred @ succ @ 0b00000 @ 0b000 @ 0b00000 @ 0b0001111

I note that you used 0001, when the spec defines FENCE.TSO with fm=0b1000, but then I noticed that the IDL does the same (0001):

  if (fm == 1) {
    if (pred == 0x3 && succ == 0x3) {
      # this is a fence.tso instruction
      is_fence_tso = true;
    }
  }

Is the IDL code incorrect there? If we force FENCE to have fm=0b0000, then all the tests for values of fm go away?

@AFOliveira
Copy link
Collaborator Author

LLVM also represents fm as a hardcoded value. I re-opened my issue on the ISA manual to see if I can get clarification on this (riscv/riscv-isa-manual#1782 (comment))

I was wrong and I think you caught an IDL error. Indeed, correct value is described in the ISA as fm=0b1000

Image

@dhower-qc
Copy link
Collaborator

Yep, that's an IDL bug.

@dhower-qc dhower-qc linked a pull request Feb 11, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants