From 69b3f68597a483aa4b8f91f7c0d52e8fd59b9d78 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Fri, 24 Jun 2022 00:32:46 -0400 Subject: [PATCH] [FIRRTL] Fix bug in GCT Taps XMRs (#3415) Fix a bug in Grand Central (GCT) Data/Mem Taps pass where Data Taps could be produced with invalid XMRs. An exhaustive test case is added that checks all permutations of upwards and downwards XMRs including corner cases where the tap module is a leaf and the root. Fixes #3414. --- .../FIRRTL/Transforms/GrandCentralTaps.cpp | 43 ++-- .../FIRRTL/SFCTests/data-taps-nlas.fir | 191 ++++++++++++++++++ 2 files changed, 206 insertions(+), 28 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp b/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp index c0c2b11a363e..c2e5e354ec24 100644 --- a/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp +++ b/lib/Dialect/FIRRTL/Transforms/GrandCentralTaps.cpp @@ -830,9 +830,14 @@ void GrandCentralTapsPass::runOnOperation() { << " - Shortest prefix " << *shortestPrefix << "\n"); // Determine the module at which the hierarchical name should start. - Operation *opInRootModule = - shortestPrefix->empty() ? path.front() : shortestPrefix->front(); - auto rootModule = opInRootModule->getParentOfType(); + FModuleLike rootModule; + if (shortestPrefix->empty()) { + if (port.target.hasPort()) + rootModule = cast(port.target.getOp()); + else + rootModule = port.target.getOp()->getParentOfType(); + } else + rootModule = shortestPrefix->front()->getParentOfType(); SmallVector symbols; SmallString<128> hname; @@ -847,36 +852,18 @@ void GrandCentralTapsPass::runOnOperation() { // Concatenate the prefix into a proper full hierarchical name. addSymbol( FlatSymbolRefAttr::get(SymbolTable::getSymbolName(rootModule))); - if (port.nla && shortestPrefix->empty() && - port.nla.root() != rootModule.moduleNameAttr()) { - // This handles the case when nla is not considered for common prefix - // stripping. - auto rootMod = port.nla.root(); - for (auto p : path) { - if (rootMod == p->getParentOfType().getNameAttr()) - break; - addSymbol(getInnerRefTo(p)); - } - } for (auto inst : shortestPrefix.getValue()) addSymbol(getInnerRefTo(inst)); - if (port.nla) { - auto namepath = port.nla.namepath(); - Attribute leaf = namepath.getValue().back(); - if (!leaf.isa()) { - if (port.target.hasPort()) - leaf = getInnerRefTo(port.target.getOp(), port.target.getPort()); - else - leaf = getInnerRefTo(port.target.getOp()); - } - addSymbol(leaf); - } else if (port.target.getOp()) { + + if (port.target.getOp()) { + Attribute leaf; if (port.target.hasPort()) - addSymbol( - getInnerRefTo(port.target.getOp(), port.target.getPort())); + leaf = getInnerRefTo(port.target.getOp(), port.target.getPort()); else - addSymbol(getInnerRefTo(port.target.getOp())); + leaf = getInnerRefTo(port.target.getOp()); + addSymbol(leaf); } + if (!port.suffix.empty()) { hname += '.'; hname += port.suffix; diff --git a/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir b/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir index 9342f30822ba..67dff3ff0542 100644 --- a/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir +++ b/test/Dialect/FIRRTL/SFCTests/data-taps-nlas.fir @@ -82,3 +82,194 @@ circuit TestHarness : %[[ ; CHECK: module DataTap ; CHECK-NOT: endmodule ; CHECK: assign _0 = Top.test.signal; + +; // ----- + +circuit Top : %[[ + { + "class":"sifive.enterprise.grandcentral.DataTapsAnnotation", + "blackBox":"~Top|DataTap_Submodule", + "keys":[ + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT/submodule:Submodule>wire_Submodule", + "portName":"~Top|DataTap_Submodule>_0" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT>wire_DUT", + "portName":"~Top|DataTap_Submodule>_1" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top>wire_Top", + "portName":"~Top|DataTap_Submodule>_2" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT/submodule:Submodule>port_Submodule", + "portName":"~Top|DataTap_Submodule>_3" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT>port_DUT", + "portName":"~Top|DataTap_Submodule>_4" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top>port_Top", + "portName":"~Top|DataTap_Submodule>_5" + } + ] + }, + { + "class":"sifive.enterprise.grandcentral.DataTapsAnnotation", + "blackBox":"~Top|DataTap_DUT", + "keys":[ + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT/submodule:Submodule>wire_Submodule", + "portName":"~Top|DataTap_DUT>_0" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT>wire_DUT", + "portName":"~Top|DataTap_DUT>_1" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top>wire_Top", + "portName":"~Top|DataTap_DUT>_2" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT/submodule:Submodule>port_Submodule", + "portName":"~Top|DataTap_DUT>_3" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT>port_DUT", + "portName":"~Top|DataTap_DUT>_4" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top>port_Top", + "portName":"~Top|DataTap_DUT>_5" + } + ] + }, + { + "class":"sifive.enterprise.grandcentral.DataTapsAnnotation", + "blackBox":"~Top|DataTap_Top", + "keys":[ + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT/submodule:Submodule>wire_Submodule", + "portName":"~Top|DataTap_Top>_0" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT>wire_DUT", + "portName":"~Top|DataTap_Top>_1" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top>wire_Top", + "portName":"~Top|DataTap_Top>_2" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT/submodule:Submodule>port_Submodule", + "portName":"~Top|DataTap_Top>_3" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top/dut:DUT>port_DUT", + "portName":"~Top|DataTap_Top>_4" + }, + { + "class":"sifive.enterprise.grandcentral.ReferenceDataTapKey", + "source":"~Top|Top>port_Top", + "portName":"~Top|DataTap_Top>_5" + } + ] + } +]] + extmodule DataTap_Submodule : + output _0 : UInt<1> + output _1 : UInt<1> + output _2 : UInt<1> + output _3 : UInt<1> + output _4 : UInt<1> + output _5 : UInt<1> + + + extmodule DataTap_DUT : + output _0 : UInt<1> + output _1 : UInt<1> + output _2 : UInt<1> + output _3 : UInt<1> + output _4 : UInt<1> + output _5 : UInt<1> + + extmodule DataTap_Top : + output _0 : UInt<1> + output _1 : UInt<1> + output _2 : UInt<1> + output _3 : UInt<1> + output _4 : UInt<1> + output _5 : UInt<1> + + module Submodule : + output port_Submodule: UInt<1> + port_Submodule is invalid + + wire wire_Submodule: UInt<1> + wire_Submodule is invalid + + inst tap of DataTap_Submodule + + module DUT : + output port_DUT: UInt<1> + port_DUT is invalid + + wire wire_DUT: UInt<1> + wire_DUT is invalid + + inst submodule of Submodule + + inst tap of DataTap_DUT + + module Top : + output port_Top : UInt<1> + port_Top is invalid + + wire wire_Top: UInt<1> + wire_Top is invalid + + inst dut of DUT + inst tap of DataTap_Top + + ; CHECK: module DataTap_Submodule + ; CHECK: assign _0 = Submodule.wire_Submodule + ; CHECK-NEXT: assign _1 = DUT.wire_DUT + ; CHECK-NEXT: assign _2 = Top.wire_Top + ; CHECK: assign _3 = Submodule.port_Submodule + ; CHECK-NEXT: assign _4 = DUT.port_DUT + ; CHECK-NEXT: assign _5 = Top.port_Top + + ; CHECK: module DataTap_DUT + ; CHECK: assign _0 = DUT.submodule.wire_Submodule + ; CHECK-NEXT: assign _1 = DUT.wire_DUT + ; CHECK-NEXT: assign _2 = Top.wire_Top + ; CHECK: assign _3 = DUT.submodule.port_Submodule + ; CHECK-NEXT: assign _4 = DUT.port_DUT + ; CHECK-NEXT: assign _5 = Top.port_Top + + ; CHECK: module DataTap_Top + ; CHECK: assign _0 = Top.dut.submodule.wire_Submodule + ; CHECK-NEXT: assign _1 = Top.dut.wire_DUT + ; CHECK-NEXT: assign _2 = Top.wire_Top + ; CHECK: assign _3 = Top.dut.submodule.port_Submodule + ; CHECK-NEXT: assign _4 = Top.dut.port_DUT + ; CHECK-NEXT: assign _5 = Top.port_Top