search

riscv-software-src / riscv-isac

BSD 3-Clause "New" or "Revised" License
32 stars 55 forks source link

New sanitize function has to be derived for "fcvt.d.s", "fcvt.d.w" and "fcvt.d.wu" #51

Open ptprasanna opened 2 years ago

ptprasanna commented 2 years ago

https://github.com/riscv-software-src/riscv-isac/blob/ba72cb7daf8ff4b00bc3c7eb33f27349ee78b90c/riscv_isac/fp_dataset.py#L37

The nan boxed value returned by sail is 0x7FF8000000000000 which is assumed to be valid with respect to the spec "Any operation that writes a narrower result to an f register must write all 1s to the uppermost FLEN−n bits to yield a legal NaN-boxed value". But the generated rs_nan_prefix value 0xffffffff (by riscof) is not matching up the extracted nan prefix from the sail log which is 0x0

This has to addressed for each of the fcvt function given below, as the nan boxed value are different for each one of them. Above value are given as an example from fcvt.d.s logs.

fcvt.d.s
fcvt.d.w
fcvt.d.wu
pawks commented 2 years ago

I think you are confusing the result and source operands of the operation. The instructions you have listed out result in a double precision result(from a single precision value in the register). So the result you see isn't actually a nan-boxed single precision value, it is infact a valid double precision value.

ptprasanna commented 2 years ago

The result may be a valid double precision value as we are doing single to double conversion.The main issue here is the derived nan-prefix (again why are we deriving nan-prefix? - Assuming it's because the flen is grater than iflen) from 0x7FF8000000000000, is not same as the nan-prefix that we generate as part of sanitize functions, or vice versa.

The other argument of nanboxing - The double-precision value may be a nan-boxed single-precision value. In this case if it is not nan-boxed then, we shouldn't be verifying the nan-prefix at all, so as the cover points shouldn't be having nanprefix values. But the present lambda function verifies if flen and iflen are not equal, then add nan-prefix which needs to be amended.

Hope one of the above does making sense.

pawks commented 2 years ago

The nan-prefixes are checked on the inputs and not the outputs. The sources here are correctly nan-boxed in the data section(here). The the nan-prefix is still applicable here since the source is a sp value and it has to be nan-boxed. Yes, the resulting dp value can be a valid nan-boxed sp value, but that does not have any relevance to the coverpoints.

pawks commented 2 years ago

The coverage for the fcvt instructions cannot be 100% due to the limitations of the compiler. Ref here. The rm in the instruction is hardcoded to 0, while the coverpoints expect them to be 7.

allenjbaum commented 2 years ago

Is this a test we should/could hand write, using explicit .fill, or by by doing self-modifying code just to test these?

pawks commented 2 years ago

We definitely could handwrite those tests using .fill but I am not sure we should.

allenjbaum commented 2 years ago

What is the alternative? I don't think that getting them to change to toolchain is practical, and the other option is to not test at all.

The spec says that FCVT.D.W[U} is unaffected by RM, and that FCVT.D.S will never round (which are really the same thing.) There are 5 defined rounding mode, + dynamic, which can encode any of those 5, so 10 possibilities in total. These tests should primarily ensure that they don't trap, I think. Testing that they don't round doesn't make sense since they could round but the answer wouldn't change.

Self modifying code would be easier, and there's. a precedent in the trap handler (in the prolog if xTVEC isn't writable and in the GOTO_MMODE macro) You could argue that there shouldn't be (e.g we could require implementers to supply the address of a writable, executable code that could be loaded into xTVEC) though.

On Mon, Sep 26, 2022 at 2:36 AM S Pawan Kumar @.***> wrote:

We definitely could handwrite those tests using .fill but I am not sure we should.

— Reply to this email directly, view it on GitHub https://github.com/riscv-software-src/riscv-isac/issues/51#issuecomment-1257763834, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJQB5IHRXFQJRP5M6S3WAFVBLANCNFSM6AAAAAAQMDSYNA . You are receiving this because you commented.Message ID: @.***>

pawks commented 2 years ago

I definitely think that the toolchain should support all the fields as legally allowed by the spec. Especially since this does not come with any negative side effects. The fix for this is pretty simple in the assembler. These instructions will never be generated from the C code and only hand written assembly will have these instructions. So, I definitely think the toolchain should support this.

The other alternative is to modify the coverpoints to not consider the rm_val itself. Even though this is okay, but it degrades the quality of the coverpoints. The coverpoints currently only look for rm_val==7, if we modify it to rm_val==0 then the coverage is 100% but that does not address all the other cases. So even if we try to write the tests for those, we wouldn't be able to compile those. I am not in favor of self modifying code, especially in places where it is not required. By adding self-modifying code we increase the minimum PMA attribute requirements for the code section to run these tests.

The final alternative is using .fill to hand write these tests. These tests can be written and used intermittently but I don't think this is a long term solution for the problem.

allenjbaum commented 2 years ago

I don't think it is practical to get the toolchain modified to produce an encoding that compilers will never generate, and will only be used by tests - and never in real code. I got strong pushback when I asked for it.

This case is not common, and while the .fill solution is not a long term solution, I'm not sure it needs to be. This case isn't going to come up that often (as opposed to when those encodings are "reserved", which is more typical)

On Tue, Sep 27, 2022 at 12:01 AM S Pawan Kumar @.***> wrote:

I definitely think that the toolchain should support all the fields as legally allowed by the spec. Especially since this does not come with any negative side effects. The fix for this is pretty simple in the assembler. These instructions will never be generated from the C code and only hand written assembly will have these instructions. So, I definitely think the toolchain should support this.

The other alternative is to modify the coverpoints to not consider the rm_val itself. Even though this is okay, but it degrades the quality of the coverpoints. The coverpoints currently only look for rm_val==7, if we modify it to rm_val==0 then the coverage is 100% but that does not address all the other cases. So even if we try to write the tests for those, we wouldn't be able to compile those. I am not in favor of self modifying code, especially in places where it is not required. By adding self-modifying code we increase the minimum PMA attribute requirements for the code section to run these tests.

The final alternative is using .fill to hand write these tests. These tests can be written and used intermittently but I don't think this is a long term solution for the problem.

— Reply to this email directly, view it on GitHub https://github.com/riscv-software-src/riscv-isac/issues/51#issuecomment-1259070015, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJXEP4GGU55JV7ROS6DWAKLTVANCNFSM6AAAAAAQMDSYNA . You are receiving this because you commented.Message ID: @.***>