bucaps/marss-riscv

The bitwise operator `or` is used in a logical expression

aka-sps opened this issue · 3 comments

marss-riscv/src/riscv_cpu.c

Lines 861 to 862 in a7fac76

sd = ((val & MSTATUS_FS) == MSTATUS_FS) |
((val & MSTATUS_XS) == MSTATUS_XS);

gktrk commented

This is accurate. The RISC-V Privileged ISA spec in Section 3.1.6.5 states that:

The SD bit is a read-only bit that summarizes whether either the FS field or XS field signals the
presence of some dirty state that will require saving extended user context to memory.

It is a single bit field in mstatus. The encoding for the dirty state for both FS and XS is 11b. Using a logical OR or a bitwise OR makes no difference in terms of the output. Eventually, the sd bit is set using

if (sd)
  val |= (target_ulong)1 << (s->cur_xlen - 1);

It may look confusing but this expression is closer to how a hardware would implement it.

It would seem that logical operations are more suitable for BOOL types than operations of bit arithmetic.
In 20 years of existence of the C99 standard, you can already start using the bool type from<stdbool.h>.
Logical operations are "lazy" and are more consistent with programming.
And half of the parentheses in the expression can also be removed.

How is this implemented in hardware? On the "short path". Any channel "dirty" signal sets SD even if "x" is in other channels.

Thanks for your quick answer to the essence of the question, but since then, in addition to the FS and XS fields, a VS field has already been introduced for the vector extension "V".

A vector context status field, VS, is added to mstatus[10:9] and shadowed in sstatus[10:9]. It is defined analogously to the floating-point context status field, FS.

Now you will need three "bitwise and", three comparison and two "bitwise or"...

gktrk commented

It would seem that logical operations are more suitable for BOOL types than operations of bit arithmetic.
In 20 years of existence of the C99 standard, you can already start using the bool type from<stdbool.h>.
Logical operations are "lazy" and are more consistent with programming.
And half of the parentheses in the expression can also be removed.

Without a doubt this statement could have written in a variety of ways and styles. If I were the one who originally wrote it, I would have used a logical or as well. That line is from the original tinyemu code. Given how bitwise or of two non-zero values will not yield a zero, the statement will execute as intended. We worked very diligently when forking TinyEMU to minimize the changes to the original source files as much as possible, so that we can port fixes and features from the future releases of TinyEMU. I see your point but for practical reasons, I see no need to change it.

How is this implemented in hardware? On the "short path". Any channel "dirty" signal sets SD even if "x" is in other channels.

2 two-bit comparators to compare FS and XS against 11b and single two-input OR gate to write the output to SD was what I had in mind.

Thanks for your quick answer to the essence of the question, but since then, in addition to the FS and XS fields, a VS field has already been introduced for the vector extension "V".

A vector context status field, VS, is added to mstatus[10:9] and shadowed in sstatus[10:9]. It is defined analogously to the floating-point context status field, FS.

Now you will need three "bitwise and", three comparison and two "bitwise or"...

I'm sure when we get to support the V extension, we can write it in a more efficient way. Like I said, keeping it as-is serves a more practical purpose at the moment.