ijor / fx68k

FX68K 68000 cycle accurate SystemVerilog core
GNU General Public License v3.0
135 stars 31 forks source link

Usage of synthesis on / off #12

Open udif opened 1 year ago

udif commented 1 year ago

With respect to the following comments:

    // Simulation problem
    // Sometimes (like in MULM1) DBH is not set. AU is used in these cases just as a 6 bits counter testing if bits 5-0 are zero.
    // But when adding something like 32'hXXXX0000, the simulator (incorrectly) will set *all the 32 bits* of the result as X.

If you look at IEEE1800-2017, you will see the following, in section 11.4.3 Arithmetic operators :

For the arithmetic operators, if any operand bit value is the unknown value x or the high-impedance value z, then the entire result value shall be x.

That means that although in your case you naturally expect the low 16 output bits to not be affected by Xs on the upper 16 bits, this is not what the SystemVerilog spec says, so this is not a simulator bug, but more like a SystemVerilog gotcha.

Other than that, I would avoid the use of `synthesis translate_off altogether, and replace them with an `ifdef ... `endif:

  1. They are not defined by the language spec
  2. They do not nest.
  3. (My personal opinion) They are ugly, because we should never carry information in comments that cannot be ignored. (but it's OK to put optimization hints, for example, because you can safely ignore it).
ijor commented 1 year ago

Hi udif,

Interesting, I wasn't aware about that System Verilog specification. Thanks for letting me know. So more than a simulator bug it indeed seems to be a SV quirk. Still, you could expect that, at least, commercial simulators would offer a configuration option to change the behaviour, and avoid unneeded differences with real synthesis, but I digress.

Yeah, I know that the translate_off directive is not a SV standard. But using ifdef is not a practical solution because there is no standard predefined macro to distinguish between simulation and synthesis. That means the user would require adding an external macro somehow. I don't like that solution.

udif commented 1 year ago

By the standard, SYNTHESIS is predefined for all synthesis tools:

https://support.xilinx.com/s/question/0D52E00006lLh7zSAC/does-vivado-define-word-synthesis-automatically-how-can-i-undefine-it-in-project?language=en_US

jotego commented 1 year ago

The SYNTHESIS macro seems a more generic solution, as it is defined by the standard.

ijor commented 1 year ago

By the standard, SYNTHESIS is predefined for all synthesis tools:

Doesn't seem so. Quartus, at least some versions, doesn't support this.

udif commented 1 year ago

The quote above, is not 100% accurate. I mean it is accurate, but the citation is not from IEEE1364-2005, but rather from the withdrawn IEEE1364.1-2002 IEEE Standard for Verilog RTL.

Section 6.2 and 6.3 says two things:

  1. You should use `ifdef SYNTHESIS
  2. All comment-based pragmas are deprecated and should not be used, and they explicitly mention translate_on and translate_off

Personally, I haven't seen new code using this "feature" for the last 15 years, and I work with SystemVerilog daily.

ijor commented 1 year ago

Unfortunately it really doesn't matter here, if it is, or it was, a standard feature. As long as Quartus doesn't support this, I am afraid I cannot use it.