adding search path for undefined modules

[jankcorn]

Mike, I don't know if this is 'out of scope' for your project, but...

I am quite interested in using slang to produce a json tree for a fully synthesized project. Unfortunately, with the '-single-unit' command line flag, I would need to preprocess the source first, to find out all modules from different directories that are pulled in by the build.

For discussion purposes only, I have attached a pull request that shows the general (albeit broken!) amount of code that I believe is necessary to do this (this patch works on my source files, except for problems caused by adding multiple parse trees).

Would you be interested in a adding this functionality to slang (of course, I can do the pull request)? If so, I was thinking that the edit really needs to go into SyntaxTree::create, but it would require some surgery on the Preprocessor and Parser objects (to temporarily stop the parse at EOF and then restart after the missing modules are added). This feels a bit cumbersome to me; alternatives I have investigated are: 1) adding code to Scope::addMembers, 'case SyntaxKind::HierarchyInstantiation' or 2) adding code to tools/driver/driver.cpp. Alternative (1) above feels like an even more grievous breaking of abstractions; Alternative (2) runs into registration of multiple syntax trees, which has serious problems with macro registration (they are per Preprocessor instance) and SV 'struct' definitions in header files, which also seem to have something like a single syntax tree scope (not visible to later parsed trees).

In any case, I would be quite grateful to hear your views.

Thanks very much both for creating this package and for publishing it under MIT license! jca

[MikePopoloski]

I think this is a reasonable feature to add. It would be pretty easy if you didn't also need the "single unit" behavior for the discovered files (as an aside, I still have no idea why so many Verilog projects decide to set up their projects this way, it seems like it only has downsides).

This definitely can't happen inside Scope; once elaboration starts it's too late to add more syntax trees because there might be hierarchical references or package lookups that depend on their existence. Doing it in the Compilation isn't crazy, but I think it might be cleaner to have a separate component that can look at a bunch of SyntaxTrees and search for new files for any unknown instantiations (so essentially option 2). Each SyntaxTree has some metadata, which includes a list of hierarchical instantiations so you don't have to go searching for them again.

Once you have the newly loaded files, if you aren't in single unit mode it's easy; you just add them to the compilation like any other compilation unit. If you need a single unit, you kind of have to hack them together (as you mentioned), and maybe copy preprocessor state out of the existing unit.

I'm actually curious if you use a commercial tool that allows preprocessor state to carry over to library files loaded on the fly like this or if it's just Verilator that supports that. Icarus says this, which I agree with:

Preprocessing Library Modules

    Icarus Verilog does preprocess modules that are loaded from
    libraries via the -y mechanism. However, the only macros
    defined during the compilation of that file are those that it
    defines itself (or includes) or that are defined in the
    command line or command file.

    Specifically, macros defined in the non-library source files
    are not remembered when the library module is loaded. This is
    intentional. If it were otherwise, then compilation results
    might vary depending on the order that libraries are loaded,
    and that is too unpredictable.

    It is said that some commercial compilers do allow macro
    definitions to span library modules. That's just plain weird.

I can look into adding this relatively soon I think. Improving support for libraries is on my todo list.

[jankcorn]

The tools that I use: verilog/vivado/quartus all have this 'weird' behavior. I will try to get a license to Synopsis and verify it is the same. Although I agree with Icarus' comment about it being unstable, it shows up the the following situation: If you have an interface 'foo' between 2 modules, which are separate files, then the only mode I have been able to find is to declare it in a common file, say bar.h, and include it in each module:

bar.h:

ifndef __BAR_H__ define BAR_H interface foo; ... endinterface `endif //BAR_H

I this way, both modules share the same parsed textual definition of 'interface foo' and I don't get the 'duplicate declaration' error I would get w/o the 'ifndef'. (since symbols seem to be persistent) Checking just now, both verilator, vivado, slang all issue this error when the ifndef is omitted.

Perhaps there are better ways of doing this; if you have any ideas, I would be happy to try them. :-) (just copied this from the style I have used over the years C/C++).

I definitely agree that multiple SyntaxTrees is a reasonable way to go and then it makes handling the lookup in tools/driver/driver.cpp quite easy. For discussion, I have updated the pull request to reflect this style of processing. Note: the code is a bit crappy, since I keep track of module names locally, using an std::map (in the variable moduleMap) rather than looking them up in the definition list.

Thanks very much for looking at this! jca

[MikePopoloski]

Note that interface definitions, like modules, are visible across compilation units. So you don't have to include them into each compilation unit, you can just instantiate them. You put typically put them in their own .sv file like any other module.

[jankcorn]

Indeed, but the problem then comes back to the 'ordering' issue that came up with macro definition files.

If the interface defs are in their own .sv file, how do we write separate files implementing modport client and server(quite a common situation). In the past, I used a scheme that forced the migration of all possible interfaces up to the top level sv file, which didn't feel very satisfactory and was a hassle to maintain.

I am not trying to be 'nitpicky'.... I would really like to know a good, scalable solution to this problem! I would be interested in interface defs/implementation for libraries that can be used across a lot of projects, much as C/C++ libraries are today, so requirements of 'this file must be in the top level source' get to be a real barrier to adoption.

Thanks very much for sharing your thoughts/suggestions about this! jca

On 12/25/20, Michael Popoloski notifications@github.com wrote:

Note that interface definitions, like modules, are visible across compilation units. So you don't have to include them into each compilation unit, you can just instantiate them. You put typically put them in their own .sv file like any other module.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/MikePopoloski/slang/pull/372#issuecomment-751311140

[jankcorn]

I apologize! Thinking more about your last email, I split out each 'interface' definition into its own .sv file, leaving the typedefs in the header file. It now compiles fine, even with the 'file local' pattern for ifdefs. (and works for verilator/vivado as well) You are completely correct.

I updated the pull request to remove the change to 'macros' in Preprocessor.h

Thanks again for all your help! jca

[MikePopoloski]

Ah great. The more I thought about this last night the more convinced I became that sharing preprocessor state for files loaded on the fly is undesirable. At least when you do it for files listed explicitly on the command line you know the ordering -- for files that get searched for and added by the tool automatically, you don't even necessarily know the order they will be added in, which means subtle changes to your code can lead to files loading in different orders and suddenly macros that were previously found no longer are. Very confusing errors are sure to ensue.

Anyway, your PR is vaguely the right idea but will need a bunch of work to be production quality. If you want to do that work I can help provide feedback, or if you'd rather wait for me to do it I'm happy to work on the feature relatively soon.

[jankcorn]

I would be happy to do the work, if you can recommend a style that fits with the rest of slang.

Perhaps it would be good if you could make the changes to the 'include' directory and I can backfill/test the method bodies/etc. (Waiting is fine as well, now that I have a workaround in my local version)

The only (trivial) point I would be concerned about is to add the search directories to the command line using '-y', keeping compatible with iverilog/verilator/vivado/etc.

On 12/26/20, Michael Popoloski notifications@github.com wrote:

Ah great. The more I thought about this last night the more convinced I became that sharing preprocessor state for files loaded on the fly is undesirable. At least when you do it for files listed explicitly on the command line you know the ordering -- for files that get searched for and added by the tool automatically, you don't even necessarily know the order they will be added in, which means subtle changes to your code can lead to files loading in different orders and suddenly macros that were previously found no longer are. Very confusing errors are sure to ensue.

Anyway, your PR is vaguely the right idea but will need a bunch of work to be production quality. If you want to do that work I can help provide feedback, or if you'd rather wait for me to do it I'm happy to work on the feature relatively soon.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/MikePopoloski/slang/pull/372#issuecomment-751355809

[MikePopoloski]

I added -y and -Y options (--libdir and --libext) to implement this feature.

[jankcorn]

Works perfectly. Thanks very much!

On 1/1/21, Michael Popoloski notifications@github.com wrote:

Closed #372.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/MikePopoloski/slang/pull/372#event-4161418653