Slightly modernize compiler Verilog output

[thoughtpolice]

This fixes the Verilog output, as described in #118, and makes Yosys happy with the output from the bsc compiler. It basically moves bsc into the realm of emitting Verilog 2001, which I think is an acceptable state of affairs.

As explained in my comment in #118 located here, and basically: I think dropping psuedo Verilog 95 support and the -v95 flag is probably the right thing to do (and more broadly I think bsc shouldn't really try to please users of totally decrepit synthesis tools-that-only-accept-v95-but-probably-still-cost-10-billion-dollars-a-year, and alternative options like using Yosys to do this should be explored, but anyway...) However, this patch doesn't include that change or remove any of that code. I can add it if it seems reasonable, though.

[czeck]

I like the moving towards a more modern verilog as well as the move away from the synopsys pragmas from the early 90's. The changes always combinational blocks looks good. Vivado 2020.2 seems to like them.

bsc generates codes like the following.

  `ifdef YOSYS
  `elsif BSV_NO_INITIAL_BLOCKS
  `else
  // synopsys translate_off

We should not add 'ifdef for YOSYS or any other 3rd party tool in generated code, setting BSV_NO_INITIAL_BLOCKS accompished the same.
synopsys translate_off should be replace with a pragma.

It is probably past time to deprecate the v95 flag.

[thoughtpolice]

Ah, I forgot about the ifdef YOSYS part. Removing it shouldn't be a problem.

I can also work on removing the v95 flag, too, I suppose. It's probably best to just delete it before making any new releases...

[kammoh]

Hi!
I also had started experimenting on this here
I've been running the generated verilog for a few designs through different FPGA/ASIC flows, mostly by using xeda, and checking compatibility, correctness (through post synthesis simulation) and capturing any potential QoR changes.
I was simultaneously interested in adding adding default reset polarity to the generated code, as well as the removal of initialization blocks, all done on the same branch. Also didn't yet expose the any of the options as command-line flags. So just an experiment, evaluating the changes, while also trying to find my way through the bsc codebase :)

Regarding translate_off:

IEEE IEEE 1364.1 2005 at least specifies the use of SYNTHESIS maco:

A synthesis tool shall define a Verilog macro definition for the macro named SYNTHESIS before reading any
Verilog synthesis source files.

Many tools, including Yosys, Vivado, and Design Compiler support the SYNTHESIS macro.

[quark17]

I wonder if initial blocks should just be guarded by BSV_NO_INITIAL_BLOCKS and not have any pragmas. An FPGA can have initial state preloaded, so Vivado does synthesize some initial blocks. If you guarded the initial blocks with SYNTHESIZE then presumably Vivado wouldn't be able to synthesize initial blocks, even if the user wanted that. If we just guard it with the BSV macro, then users can decide which behavior they want. (I wonder if we should renamed the BSV macros to be BSC, to reference the tool and not one of the multiple syntaxes that it supports.)

[kammoh]

I agree that initial block should be made conditionally available for synthesis as well.
In addition, I'd very much like to have compiler flags which suppress generation of macro-guarded blocks, such as these initialization blocks, all together. Obviously initial blocks don't make sense for some targets and applications (e.g. ASICs) and could potentially cause mismatches between simulation and synthesis.
System task blocks on the other hand should be probably guarded with SYNTHESIS. Some synthesis tools don't like them at all.
As for naming, BSC sounds like a better choice to me too, as long as it's kept consistent.

[czeck]

I'd be content if we removed initial blocks. From a pure verilog development view, they are a crutch to avoid X propagation issues. Within bsc, they remained for legacy reasons to match simulation results with bluesim. However, the goal should be to support the various and conflicting methodologies, which include both having initial blocks and not having initial blocks. The use of BSV_NO_INITIAL_BLOCKS allows both extremes. The -verilog-filter option can give you everything you might want. Ed.

…

On Wed, Jan 20, 2021 at 5:42 PM Kamyar Mohajerani ***@***.***> wrote: I agree that initial block should be made conditionally available for synthesis as well. In addition, I'd very much like to have compiler flags which suppress generation of macro-guarded blocks, such as these initialization blocks, all together. Obviously initial blocks don't make sense for some targets and applications (e.g. ASICs) and could potentially cause mismatches between simulation and synthesis. System task blocks on the other hand should be probably guarded with SYNTHESIS. Some synthesis tools don't like them at all. As for naming, BSC sounds like a better choice to me too, as long as it's kept consistent. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#294 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB24HQGS77N4B54DSVXGD4LS25L5BANCNFSM4WFAAW4A> .

[kammoh]

The -verilog-filter option relies on the availability of an external tool (such as a Verilog pre-processor or a custom Python script) not shipped with bsc, and not necessarily available to the user. The same functionality may well be achieved by running the same external tool subsequently after bsc, without even requiring the -verilog-filter flag. Theoretically, a Verilog transformation script (run through -verilog-filter option, or simply on the generated Verilog) can realize all the enhancements proposed in this PR as well as many other enhancements and fixes!
I think giving more control over generated Verilog, without relying on external post-processors, can greatly enhance out-of-the-box experience with the compiler, especially for newcomers to Bluespec.

[thoughtpolice]

Sorry for the delay on this. Regarding all the comments on the output Verilog: I'm afraid I'm no expert in Verilog myself (I'm but a glorious, luxurious, exclusive user of alt-HDLs only, and only consider Verilog a "necessary evil"), but I agree there's definitely some design discussion to be had about the compiler output. I'll get this PR in shape so we can start thinking about the good stuff...

I have a separate (somewhat radical) proposal that might hopefully lead to a useful discussion there, too. I'll try writing something up later.

[thoughtpolice]

@quark17 Sorry for the delay, but I think this should be ready now! (I've addressed the one nit by Ed) Let me know if anything else is needed.

[quark17]

For the always-block issue: Rather than try to analyze the always-block to determine if it's a combinational always-block (which I think is going to be fragile), I would rather that this decision be specified by the place that creates the always-block. Specifically, I would change the VStmt data type to not only have a VAt constructor (which has a sensitivity list) but also to have a new VAtComb constructor (that does not carry a sensitivity list). The pretty-printer would print VAt the same way as before, and would print VAtComb with *. You'll notice that in AVerilogUtil.hs, of the five places that create expressions with VAt, four of them calculate the sensitivity list by applying aIds to the expression(s) to get the list of mentioned signals -- those places can instead create VAtComb and not have to compute the sensitivity list.

If it helps, what's happening here is that BSC has generated an ASyntax representation for a module, and the stage AVerilog converts that representation into a Verilog representation. The file Verilog.hs defines a (mostly) generic Verilog syntax and a pretty printer for it.

Note that BSC creates the combinational always-blocks in places where a continuous assignment is preferred, but Verilog does not allow writing the definition that way. For example, there is no case-expression, only case-statements, so any signal that is defined with a case-expression has to be written with an always-block. Further, BSC will declare that signal as reg, not wire, because -- as I understand -- this is required for assignment inside an always-block, even if the signal won't actually be implemented with a register/latch by synthesis tools. I have a question, then: With combinational always-blocks using *, do assigned signals still need to be declared as reg?

For translate_off: You're only addressing the translate_off pragmas on initial blocks, but there are other places where BSC can insert this pragma -- for example, around foreign function calls (whether built in system tasks or imported C functions). Instead of moving a few translate_off occurrences inside BSV_NO_INITIAL_BLOCKS, I would suggest that we leave them where they are but print them instead as ifdef/ifndef blocks, using a new macro like BSV_SYNTHESIS (or BSV_SIMULATION? or BSV_TRANSLATE_OFF?). I also note that translate_off appears in most of BSC's Verilog primitives, so it would need to be changed there. (All of this could also be accomplished with a post-processing script that blindly replaces occurrences of translate_off with ifdef BSV_SIMULATE and translate_on with endif, but I'm OK with making this default, and requiring a script to turn it back the other way.)

[wlott]

I'd be content if we removed initial blocks. From a pure verilog development view, they are a crutch to avoid X propagation issues.

They are more than that. FPGA synthesis tools use initial blocks and assignments to establish the value held by the register or BRAM immediately after configuration. In fact, the initial value for a register doesn't even need to be the same as the "reset" value. If you remove initial blocks or disable them for synthesis, you would make it impossible for FPGA targeted designs to leverage this.

[thoughtpolice]

Actually, I believe initial blocks do have another tangible advantage over resets on FPGAs in that the initial value can be encoded in the CRAM/power-on state, not using routed wires to enable a reset line. This can have the advantage that if your power on state is the same as reset (or you can activate some kind of "soft reset" through a reset controller), then you don't need any reset wires at all, which can help P&R since there will be less routing to handle, and thus less work to do. But this doesn't work on ASICs or even every FPGA I think (some Microsemi FPGAs lack initial CRAM values according to my memory)

But I don't think that's quite relevant to the compiler output, which is my focus here. This problem seems much more like an API design question. For your proposed example where reset and initial are different, you'd almost certainly want a separate mkReg2(initial, reset); API anyway for that case; bsc generated Verilog uses the same initial value for all regs in an emitted module, and it's all-or-nothing (applies to all regs, or none), etc. That seems like a very constrained scenario to take advantage of, and in any cases like this you'd almost certainly want better control: your control plane logic might want resets while your data plane might want initials only, or nothing at all, etc. Handling all this at the level of compiler output is too inexpressive and brittle for every user.

So I think there's definitely some API work that needs to be done for the FPGA families out there, but it's probably a wholly separate issue. There's definitely some stuff that could help that by being fixed in the compiler though...

[wlott]

But I don't think that's quite relevant to the compiler output, which is my focus here.

You are right: a discussion about the semantics the compiler should offer is of much broader scope than this change, which is just targeting how these particular semantics should be expressed.

If we grant that the semantics of mkReg, mkRegU, and mkRegA are that they are "initialized" in simulation only and mkReg and mkRegA must be actively reset (not just powered-on configured) even on an FPGA, we can still usefully discuss the best Verilog text to express those semantics.

To that end, your moving the "synopsys translate_" comments inside the ifdef BSV_NO_INITIAL_BLOCKS does seem like a reasonable hill-climb towards "better" to me. Not only that, but the src/Verilog/Reg.v versions all have the translate_* comments inside the ifdefs, so you could also argue that this change is just bringing the inline versions into alignment with their reference counterparts.

[quark17]

@thoughtpolice, thank you again for pushing these improvements. I am about to tag a first release of BSC, and while I would really like to include these kinds of changes (to modernize the generated Verilog and Verilog primitives), I have decided to wait and make them a priority for the next release. (I will see about starting a "Discussions" page for what goes in the next release?)

(1) The always-block commit here will need to change, according to my suggestions above. (Since I understand that code, I could just do that.)

(2) Using attributes (instead of synopsys comments) on case-statements looks good.

(3) Moving the // synopsys translate_off inside of the ifdef for initial blocks is fine. (Something about your commit didn't seem clean when I first saw it, but I can't say what, and I think maybe that is the best implementation.) Although this could be made moot by other changes...

In addition to your three commits:

(4) I support replacing synopsys translate_off comments with `ifndef SYNTHESIS directives. (And then we could remove the definitions of mkSynthPragma and synthesis_str entirely from Verilog.hs.)

(5) Whether it's inside or outside, there's still the question of whether we need `ifndef SYNTHESIS for initial blocks when there's already `ifdef `BSV_NO_INITIAL_BLOCKS. And, in fact, I wonder whether using ifdef would cause a change in behavior: I could have sworn that Vivado respects synopsys comments, but I also thought that Vivado was synthesizing initial blocks -- so I'd want to check whether I was mistaken about that.

(6) I support removing the -v95 flag.

(7) And I would want to rename the macros starting with BSV_ to start with BSC_ (since that's the tool name and we support more front-ends than just the BSV syntax).

(8) And we should make sure that these changes are all applied to the primitives, as well, not just the generated code.

I think all of these are something that we can do soon (right after tagging a first release), to be tested for when we tag a next release at the end of the year. (The plan is to tag releases in July and January; although I'm not opposed to tagging intermediate releases, if you need them.)

Some of these are significant compatibility changes (and there will be others, like renaming the Verilog primitives, that you also suggested), so we will need to make this loudly apparent in the next release. But I also wonder if the release notes for the imminent release should warn users to prepare for upcoming changes.

[thoughtpolice]

@quark17 Thanks for all the suggestions, and sorry for the delay here. I've made a few changes you've requested and opened a new PR to continue this, so I'll close this one; please see #400 and let me know what you think. Thanks!