[SV_backend] Move global signals to the sail_toplevel module

[NicolasVanPhan]

Issue

The Sail global variables are emitted as signals defined at the top level of the generated SystemVerilog (SV) code, rather than within an SV module. These signals are then referenced without a hierarchical path.

This can cause issues in some formal EDA tools, as these tools fail to resolve the references to these signals. Consequently, this leads to errors in signal connectivity and results in numerous waveform issues.

This problem is discussed in detail in Sail Issue #851.

Example

In the example below, the zero_reg signal is declared at the top level and referenced in the rX module.
However, the EDA tool cannot resolve zero_reg because it is neither declared nor driven within rX.

logic [31:0] zero_reg;

module sail_setup_let_18(
    output logic [31:0] zero_reg_1
);
    always_comb begin
        zero_reg_1 = 32'h0;
    end;
endmodule

module rX(
  /* inputs don't mention `zero_reg` */
);
 /* internal signals declarations don't mention `zero_reg` */
...
    regval_from_reg inst_0_regval_from_reg(((r_0 == 64'h0) ? zero_reg : ... /* zero_reg not driven anywhere */
...
endmodule

Proposed Fix

To resolve the issue, the following changes are suggested:

• Remove the zero_reg declaration from the top level.
• Add zero_reg as an internal signal within the sail_toplevel module.
• Update all references to zero_reg to use the hierarchical path SAIL_GLOBALS.zero_reg.

By defining a macro like define SAIL_GLOBALS hier.path.to.sail_toplevel.instance, all signals can be found under the sail_toplevel module, eliminating the unresolved hierarchical reference errors in EDA tools.

Resulting Diff:

- logic [31:0] zero_reg;

 module sail_setup_let_18(
     output logic [31:0] zero_reg_1
 );
     always_comb begin
         zero_reg_1 = 32'h0;
     end;
 endmodule

 module rX(
   /* inputs don't mention `zero_reg` */
 );
 ...
-    regval_from_reg inst_0_regval_from_reg(((r_0 == 64'h0) ? zero_reg : ... 
+    regval_from_reg inst_0_regval_from_reg(((r_0 == 64'h0) ? SAIL_GLOBALS.zero_reg : ... 
...
endmodule

module sail_toplevel ()
+ logic [31:0] zero_reg;
...
  sail_setup_let_18  sail_inst_let_18(zero_reg);
endmodule

This approach ensures all global signals are encapsulated within the sail_toplevel module, making them accessible to the EDA tools via a defined hierarchical path.

This fix can be enabled or disabled using the --sv-no-toplevel-globals flag.

Let me know if further clarification or details are required!

[github-actions]

Test Results

   12 files  ±0     24 suites  ±0   0s ⏱️ ±0s
  745 tests ±0    745 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 484 runs  ±0  2 484 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 1459a27. ± Comparison against base commit 6f3985a.

[Alasdair]

Looks like the change we want in the output, but I don't think we need to pass a prepend_globals flag through all the pretty printing functions, and we shouldn't use a global variable to track the set of global identifiers.

That does mean we need to either:

• Pass spec_info through the pretty printing functions
• Add a separate toplevel id constructor to the SVIR AST

[Alasdair]

The point of packaging all the information this function gathers into a spec_info structure is specifically to avoid the use of global state.

[Alasdair]

If we need the ctyp here, it would be cleaner to just turn global_lets into a NameMap and refactor that way rather than duplicating everything.

[Alasdair]

This should use the Sail error reporting mechanism raise (Reporting.err_general l <msg>) so it prints a message formatted in the standard way with location information.

[Alasdair]

(Although without the global variable this function shouldn't exist)

[Alasdair]

I think this prepend_globals flag is pointless because Config.no_toplevel_globals handles this.

[Alasdair]

Rather than using the prepend_globals flag and is_sv_global this function should take spec_info as an argument to get the information placed there (in line 553 of this PR).

[Alasdair]

Doesn't make sense to take prepend_globals as a parameter then immediately redefine it. I'm not sure I understand what this is trying to do anyway?

[Alasdair]

There's no need to do ?optional_argument:None in OCaml (just omit it). I'm actually very surprised this even compiles, as I thought an optional argument was only implicitly an option type if you don't give a default value.

[Alasdair]

Thinking about it more, distinguishing toplevel ids from regular ids in the abstract syntax tree is probably the right approach, and it fits most cleanly with how the code is currently set up.

[NicolasVanPhan]

Hello Alasdair !
Thanks for your quick reply,

When rewriting the IDs, we need to know in which module we currently are so we can decide whether to prepend references or not, there are 3 cases :
A. In sail_setup_let_xxx the zero_reg refers to the module input, so no prepending here.
B. In sail_toplevel the zero_reg refers to the internal signal (added in this PR), so no prepending neither.
C. Everywhere else, zero_reg is a global reference and should be SAIL_GLOBALS.zero_reg, so we prepend.

Consequently, I brought this info indirectly in the pp_id function through the prepend_globals option, which is supposed to be true everywhere except when walking the sail_setup_ and sail_toplevel modules.
However it's not really clear and quite cumbersome, as you mentioned.

I agree with separating ids vs toplevel ids with 2 different AST nodes, I think the meaning is more explicit and it also solves the above issue - in cases A and B we don't prepend simply because these are not toplevel references, and in case C they are. This way, we simply prepend all toplevel ids and don't prepend the local one and that's it, no need for prepend_globals anymore.

However I don't know where to do this separation.

1. Doing it upfront in the AST seems the easiest, since there's already the whole infrastructure to add a rewriter, but then I don't know how much code duplication it will bring downstream, in the AST->JIB->SVIR pipe (everywhere there's something done on the E_id, we'll have to copy/paste the code for E_global_id I guess, and that may be a lot of boilerplate, no ?).
2. Doing it downstream on the SVIR seems more appropriate to avoid issue in 1., but I haven't found the same rewriter infrastructure as we have for the AST. Am I missing something ? Do you have an example SVIR->SVIR rewriter I can look at for reference ?.

So, to implement your solution, do you think we should go with 1 or 2, or maybe something else ?

Many thanks