Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CheckCombLoops] Handle RWProbeOp #6824

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[CheckCombLoops] Handle RWProbeOp #6824

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebb5d13
[CheckCombLoops] Handle RWProbeOp
prithayan Mar 13, 2024
76871d9
Add examples from will
prithayan Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[CheckCombLoops] Handle RWProbeOp
  • Loading branch information
@prithayan
prithayan committed Mar 13, 2024
commit ebb5d136e7eace79a2523ca61f55705781aa85f1
64 changes: 57 additions & 7 deletions lib/Dialect/FIRRTL/Transforms/CheckCombLoops.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
#include "circt/Dialect/FIRRTL/FIRRTLVisitors.h"
#include "circt/Dialect/FIRRTL/Passes.h"
#include "circt/Dialect/HW/InnerSymbolTable.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/EquivalenceClasses.h"
#include "llvm/ADT/PostOrderIterator.h"
Expand All @@ -63,9 +64,11 @@ class DiscoverLoops {
DiscoverLoops(
FModuleOp module, InstanceGraph &instanceGraph,
const DenseMap<FModuleLike, DrivenBysMapType> &otherModulePortPaths,
DrivenBysMapType &thisModulePortPaths)
DrivenBysMapType &thisModulePortPaths, SymbolTable &symtbl,
hw::InnerSymbolTableCollection &istc)
: module(module), instanceGraph(instanceGraph),
modulePortPaths(otherModulePortPaths), portPaths(thisModulePortPaths) {}
modulePortPaths(otherModulePortPaths),
portPaths(thisModulePortPaths), irn{symtbl, istc} {}

LogicalResult processModule() {
LLVM_DEBUG(llvm::dbgs() << "\n processing module :" << module.getName());
Expand Down Expand Up @@ -103,6 +106,19 @@ class DiscoverLoops {
recordDataflow(ref, data);
recordProbe(data, ref);
})
.Case<RWProbeOp>([&](RWProbeOp rwProbe) {
hw::InnerSymTarget refTarget = irn.lookup(rwProbe.getTarget());
if (!refTarget) {
rwProbe->emitWarning("cannot resolve target innerRef");
return;
}
FieldRef ref = getRefForIST(refTarget);
if (!ref)
return;
auto data = rwProbe.getResult();
addDrivenBy({data, 0}, ref);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if the rwprobe target isn't a leaf (multiple FieldRef's)?

(not sure how this works, but do we create edge for all fields connected?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider:

FIRRTL version 3.3.0
circuit Bar :
  module Foo :
    input agg : { x: Clock, y : Clock }
    output ref : RWProbe<{ x: Clock, y : Clock }>
    output refx : RWProbe<Clock>

    define ref = rwprobe(agg)
    define refx = rwprobe(agg.x)

  module Bar :
    input clock : Clock

    inst foo of Foo
    connect foo.agg.x, clock
    connect foo.agg.y, clock

    force(clock, UInt<1>(1), foo.ref.x, read(foo.refx))

Which should report a cycle but doesn't.

recordProbe({data, 0}, ref);
})
.Case<MemOp>([&](MemOp mem) { handleMemory(mem); })
.Case<RefSendOp>([&](RefSendOp send) {
recordDataflow(send.getResult(), send.getBase());
Expand Down Expand Up @@ -329,8 +345,7 @@ class DiscoverLoops {
auto iter = rwProbeRefersTo.find(*leader);

// This should be found, but for now may not be due to needing
// RWProbeOp support. May cause missed loops involving force for now.
// https://github.com/llvm/circt/issues/6820
// RWProbeOp support.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having hard time getting GitHub to let me "suggest" this properly, but this comment, line, before, and if/return can now be deleted.

if (iter == rwProbeRefersTo.end())
return;

Expand Down Expand Up @@ -388,8 +403,8 @@ class DiscoverLoops {
FieldRef srcPort(inst.getResult(srcArgNum),
modSrcPortField.getFieldID());
bool srcPortIsForceable = false;
if (auto refResultType =
type_dyn_cast<RefType>(inst.getResult(srcArgNum).getType()))
auto srcArgType = inst.getResult(srcArgNum).getType();
if (auto refResultType = type_dyn_cast<RefType>(srcArgType))
srcPortIsForceable = refResultType.getForceable();
// RWProbes can potentially refer to the same base value. Such ports
// have a path from each other, a false loop, detect such cases.
Expand Down Expand Up @@ -421,6 +436,9 @@ class DiscoverLoops {
continue;
}
}
if (!type_isa<RefType>(srcArgType) && sinkPortIsForceable)
recordProbe(srcPort, sinkPort);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you but if you could spare a word for what this does/why that'd be swell. Thank you!

If there's a path from a non-probe to a rwprobe, record a probe edge sink -> source? I'm not sure that's right, consider:

FIRRTL version 3.3.0
circuit Bar :
  module Foo :
    input clock : Clock
    output out : Clock
    output clockProbe_bore : RWProbe<Clock>

    node n = clock

    define clockProbe_bore = rwprobe(n)
    connect out, clock

  module Bar :
    input clock : Clock
    output clockProbe : RWProbe<Clock>

    inst foo of Foo
    connect foo.clock, clock
    define clockProbe = foo.clockProbe_bore

    force(clock, UInt<1>(1), clockProbe, foo.out)

Which reports a cycle but there isn't one, forcing n is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this, I need to fix the logic to track which ports can be updated by a probe. Working on it.


addDrivenBy(sinkPort, srcPort);
}
if (setOfEquivalentRWProbes.empty())
Expand Down Expand Up @@ -615,6 +633,9 @@ class DiscoverLoops {
void dumpMap() {
LLVM_DEBUG({
llvm::dbgs() << "\n Connectivity Graph ==>";
for (auto &[x, y] : nodes) {
llvm::dbgs() << "\n node : " << getName(x) << " => ID: " << y;
}
for (const auto &[index, i] : llvm::enumerate(drivenBy)) {
llvm::dbgs() << "\n ===>dst:" << getName(i.first)
<< "::" << i.first.getValue();
Expand Down Expand Up @@ -661,6 +682,12 @@ class DiscoverLoops {
rwProbeRefersTo[rwProbeClasses.getOrInsertLeaderValue(refNode)] = dataNode;
}

void recordProbe(FieldRef data, FieldRef ref) {
auto refNode = getOrAddNode(ref);
auto dataNode = getOrAddNode(data);
rwProbeRefersTo[rwProbeClasses.getOrInsertLeaderValue(refNode)] = dataNode;
}

// Add both the probes to the same equivalence class, to record that they
// refer to the same data value.
void probesReferToSameData(Value probe1, Value probe2) {
Expand Down Expand Up @@ -694,6 +721,24 @@ class DiscoverLoops {
}
}

/// Get FieldRef pointing to the specified inner symbol target, which must be
/// valid. Returns null FieldRef if target points to something with no value,
/// such as a port of an external module.
static FieldRef getRefForIST(const hw::InnerSymTarget &ist) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a utility (this is from InferWidths looks like, hopefully it is).

Not blocking but if not addressed please file an issue and assign to me.

if (ist.isPort()) {
return TypeSwitch<Operation *, FieldRef>(ist.getOp())
.Case<FModuleOp>([&](auto fmod) {
return FieldRef(fmod.getArgument(ist.getPort()), ist.getField());
})
.Default({});
}

auto symOp = dyn_cast<hw::InnerSymbolOpInterface>(ist.getOp());
assert(symOp && symOp.getTargetResultIndex() &&
(symOp.supportsPerFieldSymbols() || ist.getField() == 0));
return FieldRef(symOp.getTargetResult(), ist.getField());
}

private:
FModuleOp module;
InstanceGraph &instanceGraph;
Expand Down Expand Up @@ -723,6 +768,9 @@ class DiscoverLoops {

/// An eqv class of all the RWProbes that refer to the same base value.
llvm::EquivalenceClasses<unsigned> rwProbeClasses;

/// Full design inner symbol information.
hw::InnerRefNamespace irn;
};

/// This pass constructs a local graph for each module to detect
Expand All @@ -734,14 +782,16 @@ class CheckCombLoopsPass : public CheckCombLoopsBase<CheckCombLoopsPass> {
void runOnOperation() override {
auto &instanceGraph = getAnalysis<InstanceGraph>();
DenseMap<FModuleLike, DrivenBysMapType> modulePortPaths;
auto symTbl = getAnalysis<SymbolTable>();
auto &instc = getAnalysis<hw::InnerSymbolTableCollection>();

// Traverse modules in a post order to make sure the combinational paths
// between IOs of a module have been detected and recorded in
// `modulePortPaths` before we handle its parent modules.
for (auto *igNode : llvm::post_order<InstanceGraph *>(&instanceGraph)) {
if (auto module = dyn_cast<FModuleOp>(*igNode->getModule())) {
DiscoverLoops rdf(module, instanceGraph, modulePortPaths,
modulePortPaths[module]);
modulePortPaths[module], symTbl, instc);
if (rdf.processModule().failed()) {
return signalPassFailure();
}
Expand Down
23 changes: 23 additions & 0 deletions test/Dialect/FIRRTL/check-comb-cycles.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,3 +1106,26 @@ firrtl.circuit "Issue6820" {
firrtl.ref.force %clock, %c1_ui1, %clockProbe, %0 : !firrtl.clock, !firrtl.uint<1>, !firrtl.clock
}
}

// -----

// Check RWProbeOp + force cycles are detected.
firrtl.circuit "Issue6820_1" {
firrtl.module private @Foo(in %x: !firrtl.uint<1>, out %y: !firrtl.uint<1>, out %clockProbe_bore: !firrtl.rwprobe<uint<1>>) {
%1 = firrtl.wire sym @sym: !firrtl.uint<1>
firrtl.strictconnect %1, %x: !firrtl.uint<1>
firrtl.strictconnect %y, %1: !firrtl.uint<1>
%0 = firrtl.ref.rwprobe <@Foo::@sym> : !firrtl.rwprobe<uint<1>>
firrtl.ref.define %clockProbe_bore, %0 : !firrtl.rwprobe<uint<1>>
}
// expected-error @below {{detected combinational cycle in a FIRRTL module, sample path: Issue6820_1.{foo.x <- foo.y <- foo.x}}}
firrtl.module @Issue6820_1(in %clock: !firrtl.clock, in %x: !firrtl.uint<1>, out %clockProbe: !firrtl.rwprobe<uint<1>>) attributes {convention = #firrtl<convention scalarized>} {
%z = firrtl.wire : !firrtl.uint<1>
%foo_x, %foo_y, %foo_bore = firrtl.instance foo @Foo(in x: !firrtl.uint<1>, out y: !firrtl.uint<1>, out clockProbe_bore: !firrtl.rwprobe<uint<1>>)
firrtl.strictconnect %foo_x, %z : !firrtl.uint<1>
firrtl.ref.define %clockProbe, %foo_bore : !firrtl.rwprobe<uint<1>>
%c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
%c0_ui1 = firrtl.constant 0 : !firrtl.uint<1>
firrtl.ref.force %clock, %c1_ui1, %clockProbe, %foo_y : !firrtl.clock, !firrtl.uint<1>, !firrtl.uint<1>
}
}
Loading