search

llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.64k stars 286 forks source link

Using SystemVerilog Interfaces in Other Interfaces #1171

Open seldridge opened 3 years ago

seldridge commented 3 years ago

I've got a situation where I'd like to have one SystemVerilog interface instantiate another. I'd like to start a discussion about how we can represent nested interfaces in the IR (as this doesn't currently seem to work).

As an example, I need to represent the following (taken from #1170):

interface sub_regBundle;
  logic [15:0] b;
  logic [15:0] a;
endinterface

interface View;
  sub_regBundle sub_regBundle();
endinterface

With the current structure of sv::InterfaceOp there are a number of problems:

  1. Each sv::InterfaceOp is both a symbol and has a symbol table. This complicates things where you need to define an interface member which needs to resolve a symbol outside of its scope. This should be able to resolve any public symbol and may motivate removing the symbol table from an InterfaceOp. (Why is there a symbol table on the InterfaceOp?)
  2. There's a bunch of missing logic related to handling this. Currently, this doesn't seem to verify correctly as interfaces aren't handled in hw::getBitWidth.

To work around all this, I'm resorting to using sv::VerbatimOp in #1170.

mikeurbach commented 3 years ago

Why is there a symbol table on the InterfaceOp?

An InterfaceOp is a symbol table so you can query the signals/modports within it, if you have a reference to the InterfaceOp. I'm not sure if this is exhaustive, but a quick grep for lookupSymbol<Interface shows some of the places we make such queries.

Currently, this doesn't seem to verify correctly as interfaces aren't handled in hw::getBitWidth

I remember wondering what the bitwidth of an interface should be when I first added it. If I recall correctly, I worked around it in EmitVerilog.cpp at the time. I can't seem to find any special casing for InterfaceOp in ExportVerilog.cpp today, so I'm not sure what became of that. Perhaps some restructuring meant we no longer need to make the getBitWidth query for interfaces. Regardless, there are definitely missing pieces.

In general, this op was defined long before the more recent discussions around the scoping of symbols and symbol tables, so I'm not suprised it is a bit of a pain point. Like many things, this has been demand driven, and we simply haven't had a use-case for interfaces within interfaces yet. Now that you're pushing on it from a new angle, we can definitely revisit this.

seldridge commented 3 years ago

Thanks for the background info @mikeurbach!

Good idea to just see how the lookupSymbols are being used.

In general, this op was defined long before the more recent discussions around the scoping of symbols and symbol tables, so I'm not suprised it is a bit of a pain point. Like many things, this has been demand driven, and we simply haven't had a use-case for interfaces within interfaces yet. Now that you're pushing on it from a new angle, we can definitely revisit this.

Yeah, I agree. We should revisit this in light of the symbol table experience now and based on newly discovered uses. I was surprised that the interface code was essentially unmodified from your original support for it like 8 months ago (which is a long time considering how fast this code base is moving).