fix(inst): make fence.tso a separate inst, fix fence bug

[dhower-qc]

fixes #428

[AFOliveira]

This likely could be merged as is without any problems, right?

[AFOliveira]

Fixes #360

[dhower-qc]

Yes, it just needs to go in after cpp_hart

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[AFOliveira]

is this something that was forgotten?

[dhower-qc]

Yes, it was forgotten. I have it ready again, but it's failing because of a mismatch with riscv-opcodes.

[AFOliveira]

Yes, it was forgotten. I have it ready again, but it's failing because of a mismatch with riscv-opcodes.

It's a mismatch with LLVM. This is supposed to happen, since our view of the information is on the ISA side, which allows several arguments (in hardware) for a fence, while the pragmatic implementation of fence in a compiler is a full constant expression, since it is the only one that the ISA clearly defines. I will make a patch so the test skips fence.

This also brings me to the question that in the future, "variable" could have a separation between "hardware-only" and "assembly". Since they not always match. This is a great effort and I think should not a be a priority for now.

[ThinkOpenly]

It's a mismatch with LLVM. This is supposed to happen, since our view of the information is on the ISA side, which allows several arguments (in hardware) for a fence, while the pragmatic implementation of fence in a compiler is a full constant expression, since it is the only one that the ISA clearly defines. I will make a patch so the test skips fence.

This also brings me to the question that in the future, "variable" could have a separation between "hardware-only" and "assembly". Since they not always match.

I'm not following completely. Is this an artifact of having "variables" (rd, rs1) that aren't actually variables? Does that imply that the definition of fence.tso should not show these fields as variables? The spec says:

The unused fields in the FENCE instructions--rs1 and rd--are reserved for finer-grain fences in future
extensions. For forward compatibility, base implementations shall ignore these fields, and standard
software shall zero these fields.

Given "shall", shall we also restrict the valid values here to only zero?

not: 1, 2, 3, [...], 31

This is a great effort and I think should not a be a priority for now.

Shall we open an issue?

[dhower-qc]

Given "shall", shall we also restrict the valid values here to only zero?

The "shall" is that the fields should be ignored, not that they are illegal. If we use not, then any non-zero value will be considered an illegal instruction.

[AFOliveira]

My view on it is that the spec leaves open to later implementations to move fence around and switch any of those values. However, from the compiler view what I suppose happened is that "shall" is enough for them to assume that every bit is 0, and if someone does a different implementation, then whoever implements that is responsible for updating the compiler

[AFOliveira]

Shall we open an issue

Sure, I will.

[AFOliveira]

Smoke test fixed in #687