B-Lang-org / bsc

Bluespec Compiler (BSC)
Other
952 stars 146 forks source link

Bluesim divide-by-zero behavior is inconsistent on arm64 #688

Open quark17 opened 7 months ago

quark17 commented 7 months ago

In testsuite/bsc.misc/divmod/ there are tests for division-by-zero behavior in Bluesim and Verilog backends. The Bluesim tests are expected to raise SIGFPE. For wide bit vectors, this is achieved by defining operator/ for the WideData type that Bluesim uses to represent such vectors. Smaller types are represented in Bluesim with uint64_t, uint32_t, uint8_t, and Bluesim relies on the operator for those types to raise SIGFPE. This happens on x86_64 architectures, but does not happen on arm64 (which allows the division to return zero). So on arm64, the behavior is different for small vectors than for wide vectors.

I'm guessing that we probably want to have Bluesim raise SIGFPE on all architectures. We could do that by replacing the / operation with a macro or something, which can be defined differently for arm64 (to check for zero and raise SIGFPE). Alternatively, I guess we could change the operator for WideData to return zero when on ARM, and thus make the behavior on ARM consistent that way (which might have less overhead than guarding every division?). Any opinions?

In the meantime, I'm going to change the testsuite to expect no error on ARM, so that the testsuite doesn't fail. GitHub now offers a macOS VM on arm64 hardware (macos-14) and I want to add a CI job that builds and tests BSC on this, but currently the testsuite has this failure.

FYI, the WideData representation (and operloaded operators) are declared in bsc/src/bluesim/bs_wide_data.h and the implementations are in bsc/src/bluesim/wide_data.cxx. It is the implementation of wide_quot_rem() that raises SIGFPE (line 1221 in wide_data.cxx).

mieszko commented 7 months ago

I'm guessing that we probably want to have Bluesim raise SIGFPE on all architectures.

Why? That certainly does not respond to anything you'd see in hardware, right?

And it's not just ARM that doesn't trap for integer division. RISC-V returns all bits set, no trap. In PowerPC the result is undefined, no trap. The C standard leaves the result undefined.

Perhaps more relevantly, SystemVerilog defines division by 0 to result is 'x, which if you squint you could argue means undefined in some sense. But either way, no trap.

I think there are two rational things to do: either match Verilog behaviour (which we can't b/c we have no x) or leave the behaviour of things like this explicitly undefined in the language spec. I'd argue for the latter.

rsnikhil commented 7 months ago

I agree with @mieszko 's suggestion to leave the behaviour of things like this explicitly undefined in the language spec.

But an intermediate point would be to wrap it in something that $displays a warning in simulation.