Sunday, April 5, 2015

On SystemVerilog Coding Conventions - Challenging the Status Quo

When I first started out I remember reading all of these nice naming conventions for SystemVerilog. For example, when developing a new verification component, we're supposed to choose a descriptive name for it, preferably as short as possible. To distinguish it from other VCs doing the same thing, we should add a prefix that's unique to our organization to the name we chose. If, for example, we would be developing an AHB VC, a recommended name for it would be vgm_ahb. For the name of the package that contains the code we should add the pkg suffix, hence it should be called vgm_ahb_pkg. Any classes we write for our VC should also contain vgm_ahb in their names. For example, our driver class would be called vgm_ahb_driver, our monitor would be called vgm_ahb_monitor, etc.

I didn't pay too much attention to this at first. As a beginner I thought that there were surely good reasons for these conventions and I just happily followed them. Besides, everyone else was doing it. If we look at UVM, the package we import is called uvm_pkg. The classes we use are called uvm_object, uvm_component, etc.

Lately I got to thinking more about this and now I'm asking myself the question "Why?". A reason we might get for why things are like this is so that we can use the ever popular "wildcard import" operator, import some_pkg::*, and still avoid naming collisions. If we try to wildcard import two constructs that have the same name, the compiler will get confused. Making sure that class names are unique means that we'll never have any problems. The jury's still out on whether wildcard imports are good or bad. I haven't yet read any justification that managed to sway me one way or the other, so this is a discussion for another time.

What we can notice, though, is there is a lot of redundancy in the names. For UVM, for example, there are uvm prefixes flying around everywhere. This includes types and enumeration literals. The C language doesn't have any scoping constructs, so everything is visible. This gives people headaches when linking, as name collisions are possible. To avoid this, C developers typically try make their names unique. In SystemVerilog, however, we have the package concept, which is very similar to Java's packages or C++'s namespaces.

If we were creating a UVC, following the current guidelines it might look like this:

// file: vgm_ahb_driver.svh

class vgm_ahb_driver extends uvm_driver;
  // ...
endclass
// file: vgm_ahb_monitor.svh

class vgm_ahb_monitor extends uvm_monitor;
  // ...
endclass
// file: vgm_ahb_agent.svh

class vgm_ahb_agent extends uvm_agent;
  vgm_ahb_driver drv;
  vgm_ahb_monitor mon;

  // ...
endclass
// file: vgm_ahb_pkg.sv

package vgm_ahb_pkg;
  import uvm_pkg::*;

  `include "vgm_ahb_driver.svh"
  `include "vgm_ahb_monitor.svh"
  `include "vgm_ahb_agent.svh"
endpackage

That's a a bit too much vgm_ahb for my taste. What if we tried to shorten some of these names? Let's just cut the vgm_ahb prefix:

// file: driver.svh

class driver extends uvm_driver;
  // ...
endclass
// file: monitor.svh

class monitor extends uvm_monitor;
  // ...
endclass
// file: agent.svh

class agent extends uvm_agent;
  driver drv;
  monitor mon;

  // ...
endclass
// file: vgm_ahb.sv

package vgm_ahb;
  import uvm_pkg::*;

  `include "driver.svh"
  `include "monitor.svh"
  `include "agent.svh"
endpackage

If we were to have a testbench that instantiates an AHB agent, the code would look like this:

class env extends uvm_env;
  agent ahb_agent;

  // ...
endclass

If we were to have a second AXI UVC built in the same way, then we'd need another way to be able to differentiate between the two agents, since the class names are now identical. We can just use the scoping operator:

class env extends uvm_env;
  vgm_ahb::agent ahb_agent;
  vgm_axi::agent axi_agent;

  // ...
endclass

Why this is basically the same as using prefixes for our class names, isn't it? (There is one extra character in there because a "::" is longer than a "_", but in the grand scope of things I'd say it's negligible.) We could even go one step further and (in about 20 years) have UVM cleaned up. This would make our driver (and other component) definition(s) look like this:

// file: driver.svh

class driver extends uvm::driver;
  // ...
endclass

This frees up UVC developers to use short and descriptive names for their classes. The discussion whether to use wildcard import or not becomes orthogonal. For classes where there aren't any name collisions it's just going to work. For classes where there is one, we just use the explicitly scoped name, which is anyway (almost) as long as if we would have used the prefix.

This also applies to type definitions and enumerated literals; basically to anything that has package scope. What can't be scoped, though are macros. Those annoying `uvm_object_utils still have to contain a prefix, since they can't be defined in packages. That's ok, though, since we all agree that macros are evil and we won't use them, right? (Yeah, right!)

Going the way of no prefixes has a tiny inconvenience with the current generation of tools, though. When we compile a testbench, we very often want to do the whole compile with one tool call:

compiler +incdir+path/to/vgm/ahb vgm_ahb.sv +incdir+path/to/vgm/axi vgm_axi.sv ... 

If both packages include files called driver.svh, then for the case above we'll have the nice surprise that the AXI package includes AHB source files instead of its own. This is because the AHB directory is going to be searched before the AXI directory. We just have to separate the compile into multiple tool invocations:

compiler +incdir+path/to/vgm/ahb vgm_ahb.sv
compiler +incdir+path/to/vgm/axi vgm_axi.sv ... 

How often do we need to compile our UVCs anyway? I think this is something we could live with for now, but it would have been really great if the compiler would have automatically searched the directory where the package is located. Then we wouldn't need any funky +incdir+ arguments and this whole issue could have been averted.

Another topic I want to touch upon is those include guards we all see everywhere. I guess this is C++ legacy, where we need to include library headers to use them. The use model in SystemVerilog (similar to Java) is to import packages, though. Coming back to the example above, it doesn't make any sense for a verification environment to include the AHB driver.svh file, since that file file comes bundled with the vgm_ahb package. Trying to protect ourselves from including that file again is analogous to trying to protect ourselves from others including our cpp files in C++. Anybody who does this will be immediately informed by the compiler that something is wrong.

Coming back to macros again, that's the only place where such include guards have any place, since a macro header could get included in multiple files in the current compilation unit. I guess a lot of the confusion stems from the fact that the same svh file extension is used for both code that is intended to be included by the user (i.e. macro headers) and for code that package developers are supposed to include in their packages (as was the case above for driver.svh, monitor.svh, etc.). Maybe it would make more sense to create a new file extension for the latter case. Off the top of my head, maybe svi (SV implementation) would be a good name. Then we could publish the guideline that svh files can be included, but svi files cannot.

SystemVerulog is mixed up enough, with its design/hierarchy constructs on one side and OOP constructs on the other. The OOP constructs themselves are a confused amalgamation of C++ and Java. We can't just blindly take conventions from the latter languages and bolt them on here. We're going to need to develop our own coding conventions that make sense in the context of this language.

13 comments:

  1. Everything you said is truth! I have written this all down multiple times for multiple recipients, but never a public blog entry. So much cargo cult programming in the SV world. Thanks for taking care of it for me :-)

    The filenames issue is a sticky one. My solution is to put the package prefix on the filenames, class driver is in the file vgm_ahb_driver.sv (note it's not really a header file, so no h at the end). People get freaked out when the file doesn't have the same name as the class defined inside of it. This wouldn't be a problem if we could cleanly make the driver/monitor/scoreboard classes part of a package without doing a `include inside the package definition. To me that's the real problem: relying on the preprocessor to populate our packages. Incidentally, that would solve the unnecessary include guard problem too.

    ReplyDelete
    Replies
    1. I would have also preferred it if we could have defined what package a class/file belongs to by putting the package name in there as the first uncommented line (kind of like how namespace works in C++). On the other hand, packages are also meant for design code, so that's why the language designer probably chose to make a package an own entity (instead of a cross cutting concern). This is another example of "the worst of both worlds" we get when we try to do too much in one single language.

      Delete
  2. I shared a video blog once on this very topic: https://www.youtube.com/watch?v=cj5xeaopTQQ.
    Came to roughly the same conclusions.

    ReplyDelete
    Replies
    1. How did you handle the annoying TPRGED warning that comes whenever you have classes with the same name in different packages registered with the factory? I tried disabling it, but any code I try to add gets executed after the warnings get printed.

      Delete
    2. Found a way in the meantime. To get rid of the TPRGED warning, I qualify the type with the package:

      `uvm_object_utils(some_pkg::some_class)

      This makes the prints nicer, because now I know exactly what type I have instantiated.

      Delete
    3. Yep, you can see that in Brian's video at 2:51.

      Delete
  3. I wholeheartedly agree with your post, however I feel obliged to point out a logical error ;)

    You say "the jury's still out on whether wildcard imports are good or bad" but then go on to outline a much tidier naming convention that requires non-wildcard imports. I think you have demonstrated precisely why wildcard imports are bad and thus settled the argument.

    I'd even go one step further and claim that the standard advice to wildcard import everything is precisely what has led to the industry failing to use namespaces appropriately and thus duplicating names at every level of the hierarchy.

    A namespace polluting wildcard import is considered bad in pretty much every software language[1][2] and I''ve never understood why it's acceptable in SystemVerilog. The only explanation I can find is that SV projects are simply smaller and less complex than large software projects that require disciplined namespace separation, therefore as an industry we're slower to adopt best-practices that truly scale.


    [1] http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice
    [2] http://stackoverflow.com/questions/2386714/why-is-import-bad

    ReplyDelete
    Replies
    1. I must admit I am biased toward not using wildcard imports. As you pointed out, this is the main reason why we got into this whole prefixing mess., Structuring code like this won't prevent anyone from using it, though, as it could still be the case that nothing collides.

      Delete
  4. After seeing Brians early video i am also convinced of the merits of a strong coding style, perhaps its my VHDL hardened back ground but it definitely helps to have most of my UVCs constructed inexactly the same way.

    Although recently, I have came across a few UVM warnings from my simulator on this approach. I assume this is to do with the macros you mention above, can you confirm? E.g.

    # UVM_WARNING @ 0: reporter [TPRGED] Type name 'agent_c' already registered with factory. No string-based lookup support for multiple types with the same type name.

    I am a little stumped on a way to resolve UVM's grumbles.

    ReplyDelete
    Replies
    1. The warning you get means that you can't use 'factory.set_type_override_by_name("original_type_name", "override_type_name")'. I would anyway recommend you not use that at all as it isn't compile time safe. Better would be to use 'factory.set_type_override_by_type(original_type::get_type(), override_type::get_type())' or as I like to do it lately 'original_type::type_id::set_type_override(override_type::get_type())'. This is compile time safe as it makes sure that the types really exist and are registered with the factory using the macros (i.e. the 'type_id' member exists).

      You can't disable the warning, though, aside from maybe using a report handler.

      Delete
    2. Oddly i don't use that call anywhere i know if in my test. Its thrown out by Questasim way before the show starts happening.

      The only other strange effect i noticed happening was with my sequencers, somewhere along the way their types get confused: E.g.
      In my tb i create sequencer handles so that the tests don't need to dive down into UVC's.

      OCP_pkg::sequencer_c OCP_DMA_Patch_seqr;
      SPI_pkg::GI_sequencer_c SPI_seqr ;

      I needed to rename the type of sequencer_c for the SPI_pkg. Why? Well the simulator got confused and later on when i assigned the handle it thought that the SPI_seqr was of type OCP_pkg::sequencer_c.
      Not sure why, i suspect its a simulator fault, but i wanted to check in the context of this thread that there wasn't an implementation oversight.
      Apologies for turning this into a debug session!

      Delete
    3. You don't need to call 'set_type_override_by_name(...)' for the warning to appear. The factory will print it once you register a second type with the same name. I think registration happens when you try to create an object of that class (using ::type_id::create).

      For the second issue, it does indeed sound like a simulator problem.

      Delete
    4. To get rid of the warnings, use "package::class" as the utils macro argument. Full article on the topic here:
      http://blog.verificationgentleman.com/2015/12/packages-class-names-and-uvm.html

      Delete