Skip to content

Detect NaN in FADD and FSUB #24

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

Merged
merged 1 commit into from
Aug 2, 2022
Merged

Detect NaN in FADD and FSUB #24

merged 1 commit into from
Aug 2, 2022

Conversation

2011eric
Copy link
Collaborator

I used the pattern specified in the RISC-V ISA manual: 0x7fc00000.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List the difference of the results generated by make arch-test RISCV_DEVICE=F between the original implementation and the proposed one.

@2011eric
Copy link
Collaborator Author

2011eric commented Aug 1, 2022

Moved the definition to riscv_private.h

@jserv
Copy link
Contributor

jserv commented Aug 1, 2022

Moved the definition to riscv_private.h

Check CI regressions and attempt to fix.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "git rebase -i" to squash/rework the commits.

@jserv
Copy link
Contributor

jserv commented Aug 1, 2022

List the difference of the results generated by make arch-test RISCV_DEVICE=F between the original implementation and the proposed one.

@2011eric, you should mention the results within git commit messages.
See https://cbea.ms/git-commit/

@2011eric
Copy link
Collaborator Author

2011eric commented Aug 2, 2022

Passed 5 more tests in RV32F, listed in the commit message.

@jserv
Copy link
Contributor

jserv commented Aug 2, 2022

https://cbea.ms/git-commit/

Read How to Write a Git Commit Message carefully and enforce the rules:

  • Do not end the subject line with a period

@2011eric
Copy link
Collaborator Author

2011eric commented Aug 2, 2022

I noticed that in the expected output_signature of test fadd_b1-01, one of the results is ff800000, which should be -INF.
However, the overflow flag (OF) and inexact flag (NX) are not set.

@jserv
Copy link
Contributor

jserv commented Aug 2, 2022

I noticed that in the expected output_signature of test fadd_b1-01, one of the results is ff800000, which should be -INF. However, the overflow flag (OF) and inexact flag (NX) are not set.

You can submit issue(s) about floating-point compliance tests if you can recognize.

@2011eric
Copy link
Collaborator Author

2011eric commented Aug 2, 2022

I noticed that in the expected output_signature of test fadd_b1-01, one of the results is ff800000, which should be -INF. However, the overflow flag (OF) and inexact flag (NX) are not set.

You can submit issue(s) about floating-point compliance tests if you can recognize.

I'm not sure if I can state the problem.
In some cases, the result of the operation differs in LSB. And I don't know if it is related to rounding.
In other cases, our simulator reports zero while the golden data is 'subnormal 0' .
And in some cases, the result of FADD is correct, but the flag is set incorectly.

emulate.c Outdated
@@ -651,7 +653,7 @@ static uint32_t csr_csrrs(struct riscv_t *rv, uint32_t csr, uint32_t val)
if (!c)
return 0;

const uint32_t out = *c;
const uint32_t out = (csr == CSR_FFLAGS) ? *c & FFLAG_MASK : *c;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite as following:

uint32_t out = *c;
if (csr == CSR_FFLAGS)
    out &= FFLAG_MASK;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote the function and also add the same code in csr_csrrw and csr_csrrc.

Since FCSR is defined only if ENABLE_RV32F is defined:

static uint32_t *csr_get_ptr(struct riscv_t *rv, uint32_t csr)
{
    switch (csr) {
    case CSR_CYCLE:
        return (uint32_t *) (&rv->csr_cycle) + 0;
    case CSR_CYCLEH:
        return (uint32_t *) (&rv->csr_cycle) + 1;
    case CSR_MSTATUS:
        return (uint32_t *) (&rv->csr_mstatus);
    case CSR_MTVEC:
        return (uint32_t *) (&rv->csr_mtvec);
    case CSR_MISA:
        return (uint32_t *) (&rv->csr_misa);
    case CSR_MSCRATCH:
        return (uint32_t *) (&rv->csr_mscratch);
    case CSR_MEPC:
        return (uint32_t *) (&rv->csr_mepc);
    case CSR_MCAUSE:
        return (uint32_t *) (&rv->csr_mcause);
    case CSR_MTVAL:
        return (uint32_t *) (&rv->csr_mtval);
    case CSR_MIP:
        return (uint32_t *) (&rv->csr_mip);
#ifdef ENABLE_RV32F
    case CSR_FFLAGS:
        return (uint32_t *) (&rv->csr_fcsr);
    case CSR_FCSR:
        return (uint32_t *) (&rv->csr_fcsr);
#endif
    default:
        return NULL;
    }
}

Should I also use macro inside csr_csrrs ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I also use macro inside csr_csrrs ?

You can do it in another pull request.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ENABLE_RV32F is turned off, build system would complain:

emulate.c: In function ‘csr_csrrw’:
emulate.c:644:16: error: ‘FFLAG_MASK’ undeclared (first use in this function)
  644 |         out &= FFLAG_MASK;
      |                ^~~~~~~~~~
emulate.c:644:16: note: each undeclared identifier is reported only once for each function it appears in
emulate.c: In function ‘csr_csrrs’:
emulate.c:661:16: error: ‘FFLAG_MASK’ undeclared (first use in this function)
  661 |         out &= FFLAG_MASK;
      |                ^~~~~~~~~~
emulate.c: In function ‘csr_csrrc’:
emulate.c:678:16: error: ‘FFLAG_MASK’ undeclared (first use in this function)
  678 |         out &= FFLAG_MASK;
      |                ^~~~~~~~~~

Resolve the above compilation errors.

@jserv
Copy link
Contributor

jserv commented Aug 2, 2022

gcc-9 raises the following warnings:

emulate.c:918:15: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  918 |             *((uint32_t *) &rv->F[rd]) = RV_NAN;
      |              ~^~~~~~~~~~~~~~~~~~~~~~~~
emulate.c:930:15: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  930 |             *((uint32_t *) &rv->F[rd]) = RV_NAN;
      |              ~^~~~~~~~~~~~~~~~~~~~~~~~

@jserv
Copy link
Contributor

jserv commented Aug 2, 2022

Also, rebase the latest master branch.

Defined several floating-point exceptions and a fflag_mask in riscv_private.h
It will now detect invalid_op, overflow, and inexact  during execution of FADD and set the corresponding bit in FCSR.
Modify csr_get_ptr and csr_csrrs so that insn CSRRS can deal with CSR_FFLAGS.

New test passed:
fadd_b10-01
fadd_b11-01
fadd_b12-01
fadd_b13-01
fadd_b7-01

Add FFLAG support in csr-relative instuctions
@jserv jserv merged commit b4521fe into sysprog21:master Aug 2, 2022
vestata pushed a commit to vestata/rv32emu that referenced this pull request Jan 24, 2025
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 this pull request may close these issues.

2 participants