llvm / circt

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

[FIRRTL] ExtModule Ordering Affects File Emission #6035

Closed seldridge closed 1 year ago

seldridge commented 1 year ago

An internal user reported that BlackBoxes have started showing up in unexpected situations. The problem is that when an external module with a BlackBoxInlineAnno does not dedup for valid reasons, e.g., it has different parameters or the user doesn't run dedup, then the order in which the external modules are declared determines the file order.

Consider the following example. Here, whichever of BlackBox_1, BlackBox_2, or BlackBox_3 is declared first will determine if the blackbox is, respectively, created in the testbench/, ./ (main design/DUT), or gct/ directories.

FIRRTL version 4.0.0
circuit TestHarness: %[[
  {
    "class": "sifive.enterprise.grandcentral.ViewAnnotation",
    "name": "MyView",
    "companion": "~TestHarness|GrandCentral",
    "parent": "~TestHarness|TestHarness",
    "view": {
      "class": "sifive.enterprise.grandcentral.AugmentedBundleType",
      "defName": "Interface",
      "elements": [
        {
          "name": "uint",
          "description": "a wire called 'uint'",
          "tpe": {
            "class": "sifive.enterprise.grandcentral.AugmentedGroundType",
            "ref": {
              "circuit": "TestHarness",
              "module": "DUT",
              "path": [],
              "ref": "a",
              "component": []
            },
            "tpe": {
              "class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
            }
          }
        }
      ]
    }
  },
  {
    "class": "sifive.enterprise.firrtl.MarkDUTAnnotation",
    "target": "~TestHarness|DUT"
  },
  {
    "class": "sifive.enterprise.grandcentral.ExtractGrandCentralAnnotation",
    "directory": "gct",
    "filename": "bindings.sv"
  },
  {
    "class": "sifive.enterprise.firrtl.TestBenchDirAnnotation",
    "dirname": "testbench"
  },
  {
    "class": "firrtl.transforms.BlackBoxInlineAnno",
    "target": "~TestHarness|BlackBox_1",
    "name": "BlackBox.sv",
    "text": "// BlackBox_1\nmodule BlackBox #(parameter X=0)(output a);\nendmodule"
  },
  {
    "class": "firrtl.transforms.BlackBoxInlineAnno",
    "target": "~TestHarness|BlackBox_2",
    "name": "BlackBox.sv",
    "text": "// BlackBox_2\nmodule BlackBox #(parameter X=0)(output a);\nendmodule"
  },
  {
    "class": "firrtl.transforms.BlackBoxInlineAnno",
    "target": "~TestHarness|BlackBox_3",
    "name": "BlackBox.sv",
    "text": "// BlackBox_3\nmodule BlackBox #(parameter X=0)(output a);\nendmodule"
  },
  {
    "class":"sifive.enterprise.firrtl.NestedPrefixModulesAnnotation",
    "target":"~TestHarness|DUT",
    "prefix":"SiFive_",
    "inclusive":true
  }
]]

  extmodule BlackBox_1:
    output a: UInt<1>
    defname = BlackBox
    parameter X = 1

  extmodule BlackBox_2:
    output a: UInt<1>
    defname = BlackBox
    parameter X = 2

  extmodule BlackBox_3:
    output a: UInt<1>
    defname = BlackBox
    parameter X = 3

  module TestHarness:

    inst dut of DUT
    inst bbox of BlackBox_1

  module DUT:

    wire a: UInt<1>
    invalidate a

    inst bbox of BlackBox_2
    inst grandCentral of GrandCentral

  module GrandCentral:

    inst bbox of BlackBox_3

The three possible results are:

// ----- 8< ----- FILE "testbench/BlackBox.sv" ----- 8< -----

// Generated by CIRCT firtool-1.52.0-152-gb07e210c1
// BlackBox_1
module BlackBox #(parameter X=0)(output a);
endmodule
// ----- 8< ----- FILE "./BlackBox.sv" ----- 8< -----

// Generated by CIRCT firtool-1.52.0-152-gb07e210c1
// BlackBox_2
module BlackBox #(parameter X=0)(output a);
endmodule
// ----- 8< ----- FILE "gct/BlackBox.sv" ----- 8< -----

// Generated by CIRCT firtool-1.52.0-152-gb07e210c1
// BlackBox_3
module BlackBox #(parameter X=0)(output a);
endmodule

Note: that this is ignoring a whole separate class of errors where the actual content of the BlackBoxInlineAnno annotation is not verified in any way. I placed a different comment in each of them. Put differently, it's dangerous to rely on defname with BlackBoxInlineAnno blindly. This should at least be checked that these are the same.

seldridge commented 1 year ago

The solution here is to, in the BlackBoxReader pass, process each ExtModule with the same defname together and use the "strongest" output directory. E.g., the DUT module should "win" in the above example.