MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
558 stars 122 forks source link

Coerced ports not reflected in JSON output #888

Closed AndreasLoow closed 3 months ago

AndreasLoow commented 4 months ago

I don't know if this is the intended behaviour or not, but the following module:

module foo(input a);

assign a = 1;

endmodule

generates the following warnings:

> ~/dev/slang/build/bin/slang foo.sv --ast-json tmp.json
Top level design units:
    foo

foo.sv:3:8: warning: input net port 'a' coerced to 'inout' [-Wport-coercion]
assign a = 1;
       ^
foo.sv:1:18: note: declared here
module foo(input a);
                 ^

Build succeeded: 0 errors, 1 warning

and the resulting JSON file is:

> cat tmp.json 
{
  "name": "$root",
  "kind": "Root",
  "addr": 5865718363904,
  "members": [
    {
      "name": "",
      "kind": "CompilationUnit",
      "addr": 5865718557592
    },
    {
      "name": "foo",
      "kind": "Instance",
      "addr": 5865718558200,
      "body": {
        "name": "foo",
        "kind": "InstanceBody",
        "addr": 5865718557880,
        "members": [
          {
            "name": "a",
            "kind": "Port",
            "addr": 5865718558320,
            "type": "logic",
            "direction": "In",
            "internalSymbol": "5865718546448 a"
          },
...

I.e. the a port is still an in port despite the warning message saying that the port has been coerced to an inout port.

If the JSON is supposed to reflect the input given to slang then this behaviour is correct, but if the JSON is supposed to reflect the input after the processing done by the tool, then this output is potentially suspect I think? (In my use case the tree post processing would be more useful, but I understand that in other use cases maybe the tree before processing is more useful. Maybe a flag could make sense here?) Thanks.

AndreasLoow commented 4 months ago

Just realised that the following might be somewhat related. Slang accepts the following

module mod(input foo);

assign foo = 0;

endmodule

module top;

logic bar;

mod mod(.foo(bar));

endmodule

and gives the warning

tests/test.sv:3:8: warning: input net port 'foo' coerced to 'inout' [-Wport-coercion]

but if you modify the first line of the above code to be

module mod(inout foo);

then slang dies with

tests/test.sv:11:14: error: 'bar' cannot be connected to 'inout' port (only nets are allowed)
mod mod(.foo(bar));

so similarly to the thing I discussed in my previous comment, it seems that coercions are not remembered later. But it looks like the standard is not clear about what order the check vs. coercions should happen... and I tested a few simulators and they do the same as slang in this case.

MikePopoloski commented 4 months ago

For your first question, I can see how the message makes it seem like something is being changed ("coerced") here, but in reality the port itself still has the given direction, which mainly controls how the data flows in and out from the module itself. You've connected the port on the outside to a variable, so the value of that variable only ever flows in via continuous assignment. Then the internal net generated within the module itself is driven both by the data flowing in as well as the extra assignment (which will then follow typical multi-driven net rules to resolve a final value), so that's the thing that is actually coerced.

If you had instead connected to a net on the outside, the net would be collapsed across the hierarchy and the value would indeed flow back out even though it was an "input" port. This would currently still not result in a different AST output; you can think of the port direction as being the start of the process for determining how data should flow, not the final end state for how it does actually end up flowing. I think slang could probably represent this better in the AST but the port itself is probably not the place to do it.

Your followup is related to what I just described and I think the behavior you've observed is correct. Nothing is forgotten about the coercion; the warning about the coercion is just saying that you've multi-driven a net that you may not have meant to. The behavior otherwise should be standard across tools according to the LRM. The whole thing is weird and tricky but that's how they decided to specify things.

AndreasLoow commented 4 months ago

Interesting! The standard is very short on this as far as I know:

A port that is declared as input (output) but used as an output (input) or inout may be coerced to inout. If not coerced to inout, a warning shall be issued.

One reasonable interpretation of this, imho, is that the port itself is coerced, but it seems that tools do not do this and it's interesting that the standard text can also be read as an internal net associated with the port is coerced instead.

I guess for my use case as long as there is a way to see the final direction of the port in the JSON output, be that the port itself or some net associated with the port, everything would be OK.

MikePopoloski commented 4 months ago

I believe the LRM does say enough on the topic to work out how this should function, but they spread it around all over the place instead of stating it clearly, which is annoying, but I think you'll find that all major vendor tools will interpret this the same way. Some relevant pieces of the LRM are:

23.3.3 Port connection rules
...
Each port connection shall be a continuous assignment of source to sink, where one connected item shall be
a signal source and the other shall be a signal sink. The assignment shall be a continuous assignment from
source to sink for input or output ports. The assignment is a non-strength-reducing transistor connection for
inout ports. 
23.3.3.1 Port coercion
A port that is declared as input (output) but used as an output (input) or inout may be coerced to inout. If not
coerced to inout, a warning shall be issued. 
23.3.3.2 Port connection rules for variables
...
 An input port can be connected to any expression of a compatible data type. A continuous
assignment shall be implied when a variable is connected to an input port declaration. Assignments
to variables declared as input ports shall be illegal. If left unconnected, the port shall have the default
initial value corresponding to the data type
23.3.3.7 Port connections with dissimilar net types (net and port collapsing)
<This whole section basically>

Also see this proposed clarification to the section on Port Coercion which I found helpful, though it didn't make it into the LRM for reasons unknown: https://www.accellera.org/images/eda/sv-bc/att-7186/1573_D4_port_coercion.V2.htm

Anyway, the way you can tell this from the slang AST is to find the internal net that is generated for the port and find the drivers of that net and see that there are multiple drivers. There may not be quite enough info in the JSON to work that out (not sure), but the C++ API definitely knows all of those things.

MikePopoloski commented 3 months ago

Going to close this out because I don't think there's an action to be taken. The way to see this in the JSON dump is to look at what other drivers there are for a given port's connected net. The JSON doesn't list cross-references but I'm not sure that it should; it would greatly balloon the size of the output and you can find them yourself by walking the tree if you need to.