Slightly modernize Verilog, take 2

[thoughtpolice]

This is a continuation of #294, with the following changes at the suggestion of @quark17:

• Complete removal of the -v95 flag, so users aren't mislead. Includes a tiny bit of cleanup.
• Removal of synopsys pragmas entirely, like last time.
• But instead, we replace them with ifndef SYNTHESIS pragmas, as suggested by Julie. I found this to be a much more reasonable and less invasive change. This also avoids any changes in semantics in the output (for now.)
• We also apply this change to the entire Verilog standard library -- so downstream users don't suffer this when ingesting it.
  • I didn't apply this to the testsuite though, since I don't think there's an need.

This doesn't include the change for always @* for combinational components just yet; I'll add that in a separate PR, but I think all of these are straight-forward and uncontroversial.

[quark17]

Thank you! Apologies for the delay in reviewing! Some notes:

(1) When you make changes in GenBin and GenABin (that affect the encoding of the .bo and .ba files), you you need to also change the header string defined at the top of the file. (When BSC reads in the binary file, it checks that the string at the start of the file matches, otherwise it tells the user to recompile.) Could you force push a new version of the -v95 commit, that includes an update to the GenABin header?

(2) I'm curious about the two changes in this line of Verilog.hs:

- vi_inst_params :: Either [(Maybe String,VExpr)] [(VId, Maybe VExpr)],
+ vi_inst_params :: Either [VExpr] [(VId, VExpr)],

There are two separate changes here: removing the Maybe String on the left and the Maybe on the right. The left and right represent the two ways of writing parameters to a submodule instantation: named (on the right) and positionally/unnamed (on the left). Both are supported in Verilog 2001 and up, but V95 only supported the left (positional).

In theory, the Verilog package is a generic library that can represent Verilog programs and be printed to a file. (There's been interest in pulling out libraries like these, into their own cabal packages, to modularize the code base.) So just because BSC is no longer user some representations, doesn't mean that they have to be removed from the Verilog structure.

However, I suspect the Verilog package is already specialized to BSC in a few ways, so I have no problem if we trim out features that are unused. Specifically, we could decide to not support positional/unnamed parameters, and only generate Verilog that uses names.

However, I see that there is still one place in AVerilog that generates positional parameters, and that's used for calls to noinline functions (functions that are separately synthesized) or foreign functions (in BH/Classic, you can directly import a combinational module, which you would have to provide separately). For noinline functions, BSC is generating the parameter names (in GenFuncWrap) so BSC could record them, to use named parameters in the output; for foreign, I don't believe that the BH/Classic syntax supports specifying names, but also I don't know if it allows a type that would generate parameters (and, if it does, whether the positional order is even sensical).

Ok, so, stepping back. The Maybe String on the left in vm_inst_params is to support situations where we know the names of the parameters, but have chosen to use unnamed instantiation -- the names are then printed as a comment next to the positional arguments. It seems like you've taken the position that, if you know the names, then you should use named parameters; therefore you've deleted the option of using positional with commented names. (I don't know if this was your explicit thinking or if you were led astray by the fact that the function for printing the commented names was called pv95params. That's a misnomer, since this syntax is allowed in later Verilog, so it could be better called pvPositionalParamNames, say.)

I'm OK with that decision, although I'd prefer to go a step further: remove the positional option altogether! (Remove the Either and just have the right side.) But, as I've shown above, there are still two places that use positional parameters, and removing that use might be outside the scope of this PR. In that case, I'd like to see two things: (a) comments on vm_inst_params explaining the structure and strongly suggesting not to use the Left side, and that it should be removed; and (b) PRs opened to improve noinline and to consider what to do about foreign.

The Maybe on the right in vm_inst_params is to support the generation of a submodule instantiation where a parameter name is mentioned, but no value is provided -- which I assume is equivalent to not mentioning parameter at all, but is slightly more self-documenting, by mentioning that we know the parameter exists and have chosen to let the module use its default value. BSC doesn't use this feature, so you've removed it. As I said above, if we think this is a generically useful feature, we could decide to leave it in the Verilog library; but I'm not sure how useful it is, so it's OK if you want to remove it.

(3) FYI, there's a convention in the source code to prepend m at the start of variable names, to indicate that it's a Maybe type, particularly when we inspect the Maybe and bind its contents to a name, such that we have m<var> and then <var> (like mexpr and expr). In the pretty printing on the vm_inst_params, the right side was matched as me (maybe expression); now that you've removed the Maybe wrapper, you might want to rename that variable as just e (whereas you've left it as me in the pPrint of VMInst in Verilog.hs).

(4) No action needed, but FYI: You removed the Signed.bsv test, which was testing the difference in printing of $signed depending on the v95 flag. It seemed to me that we still want to know that BSC's codegen for signed values in $display is correct, even in our V2001+ codegen. If we weren't already testing that somewhere else, it could be worth keeping the Signed.bsv example (and moving it elsewhere). I do see that we test $display on a variety of formats, including negative numbers, in bsc.verilog/tasks/EvenMoreDisplay.bsv. So it is OK to delete Signed.bsv. Although, if you wanted to move the Signed.bsv test to that directory, I wouldn't object.

(5) Did you explicitly decide that you prefered this form:

(* parallel_case *) case (1'b1)
      foo: v = e1;
      bar: v = e2;
      ...

instead of this form (which might have been my first thought):

(* parallel_case *)
case (1'b1)
      foo: v = e1;
      bar: v = e2;
      ...

Your way has the case indented over, which bugs me. But the other way has an extra line, which also bugs me. So I'm not sure I prefer one or the other. So I'm OK with how you had it. But wondered if you'd intentionally thought about it.

(6) I see that the User Guide is inconsistent about the order of the ifdefs in its examples! You updated two examples, and they use different order. Should we fix them to both reflect the current order?

(7) There are 100+ failures in the testsuite. Most are just tests that compare the entire generated Verilog against an expected file (as an easy way to check for certain features in codegen). But there are two that are testing specifically for the pragmas around initial blocks and around system tasks. Those are in bsc.verilog/ and bsc.verilog/tasks/.

For all the tests that are just comparing the entire generated Verilog, it could be argued that those tests could be written to be more targeted in their checking. Or, since the compare procedure already runs a filter on both the generated and golden files to remove incidental differences, that the pragmas could be filtered/canonicalized in that script. But I think there's benefit to having the testsuite check the full file in lots of places, to help catch unexpected differences. So I'd say to just update the expected file in all these places. It's up to you whether you want to update each commit to include updates to the testsuite for that commit or whether you want to just add a new commit that udpates the test in one go.

Also, if you would prefer that I take any of this work over the finish line, let me know.

[thoughtpolice]

Thanks for the comments Julie, I'll try to handle these this week and reply more thoroughly when I get a chance to return to compiler hacking...