Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move global signal declaration from toplevel to the sail_toplevel module #917

Open
wants to merge 1 commit into
base: sail2
Choose a base branch
from

Conversation

NicolasVanPhan
Copy link
Contributor

This is a v2 of the former pull request : #886
Since I restarted from a new branch, I figured it would be more adequate to open a new PR.

The approach used in the v2 is :

  • Introducing a new id node for global references
    • E_gid for AST
    • AV_gid for ANF
    • V_id for JIB
    • Global_var (in addition to Var) for the SMT expression
  • Add a AST rewriter pass to change E_id global references into E_gid.
  • Remove the former prepend_globals optional argument from the pretty-printer functions
  • Augment spec_info.global_lets with the ctyp to declare the globals in module sail_toplevel (thus removing the global state introduced in the v1 of the PR).

First, a rewrite pass in the AST distinguishes global from local signals
by splitting E_id nodes into E_id or E_gid for local or global references.

Then the SV generation changes as follows :
- All signal declaration at toplevel (i.e. no parent module) are removed
- These signals are all added as 'sail_toplevel' internal signals instead
- All references to globals are prepended with the SAIL_GLOBALS macro

This prevents unresolved signal paths in EDA tools,
since now all existing signals are declared in a module.
Copy link

Test Results

   12 files  ±0     24 suites  ±0   0s ⏱️ ±0s
  757 tests ±0    757 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 501 runs  ±0  2 501 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit de18990. ± Comparison against base commit 45f2974.

@Alasdair
Copy link
Collaborator

Alasdair commented Feb 3, 2025

Hmm, I think this PR modifies too much (Not a criticism, in a way it has to for this approach)

In general our intermediate representations should have lexical scoping, because doing anything else would probably get me fired from Cambridge for crimes against tasteful programming language design (even in IRs...). In particular I don't think adding a new type of variable node to smt_exp is a good idea, but we'd kinda have to if we wanted to represent this distinction in the SV-IR, which I hadn't considered before reading this PR. That led me back to trying to fix this late in the pretty printing stage.

Could you try #953 and see if that solves your issue?

I assume the use case is something like:

module outer();
  type sail_signal;
  module sail_generated_inner();
    // needs to use 'outer.sail_signal'
  end
endmodule

A problem I have is I can't test this in CI, as Verilator doesn't support nested modules, and the only Verilator issue I could find described them as (I quote) "a language abomination that the IEEE should have known better not to add".

@NicolasVanPhan
Copy link
Contributor Author

Hello @Alasdair ,
Sorry for the late reply (I was working on some other fixes)
The change you suggested in #953 works well for me,
I rebased on it and added the remaining part adding the globals to sail_toplevel module when the option is enabled : sv_globals...NicolasVanPhan:sail:sv_globals
With your change and the remaining part, my problem is completely solved.
Do you think you'll be able to merge your suggested fix or are there other blockers ?

Many thanks !

@NicolasVanPhan
Copy link
Contributor Author

NicolasVanPhan commented Feb 10, 2025

Well actually there's an old problem I encountered again,
all global symbols are prepended with the prefix, even those in the sail_toplevel module.

However I moved the global declaration into this module, so in this module the symbols are not globals but refer to the newly declared internal signals,
so I have to find a way to not prepend global symbols in the sail_toplevel module.

However we don't have the info of the current module when we're in the pp_id function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants