[HW/Support] Introduce InstanceGraph interface #5833

mortbopet · 2023-08-13T16:02:03Z

This change moves InstanceGraphBase to support, and introduces an interface for driving the implementation, InstanceGraphInterface, which essentially is an extraction of the name and lookup related functions of HWModuleLike/HWInstanceLike (which now inherit from the InstanceGraph interfaces). The motivation for this is that multiple things in CIRCT may benefit from having graph iterators, and most/all of these will need an implementation exactly as what is given in InstanceGraphBase. My immediate usecase is to provide a graph interface for Ibis classes/containers (which don't have block argument/return values as ports, and thus aren't HWModuleLike), but things like handshake and FSM are other obvious users of this.

The interface is named InstanceGraphInterface, but in reality this is just introducing an even more generic module/instance interface than HWModuleLike - that is, instances and modules without port info. Hence, we could name the interface something more appropriate, e.g. CIRCTModuleInterface or something - LMKWYT.

Implementing this change also performs some housekeeping which bloats the PR mainly by NFCs through:

ensuring that all ops who inherit from the HWModuleLike/HWInstanceLike interfaces actually implements the name-related parts of the interface.
Various missing includes exposed by removing hw includes from InstanceGraph.

teqdruid · 2023-08-13T17:50:22Z

Haven't had a chance to review this yet, but I know @darthscsi was working on something like this #5758 and #5706.

mortbopet · 2023-08-14T03:50:55Z

@teqdruid Looks like i should read the ongoing PR list through a bit more thoroughly before starting stuff like this (to my defence, i was offline on a plane and needed something to do).
#5706 is - barring naming - virtually exactly the same as this. @darthscsi LMK if you plan on getting those two PRs in as soon as possible, or want to go with this one.

This change moves InstanceGraphBase to support, and introduces an interface for driving the implementation, `InstanceGraphInterface`, which essentially is an extraction of the name- and lookup related functions of `HWModuleLike/HWInstanceLike` (which now inherit from the `InstanceGraph` interfaces). The motivation for this is that multiple things in CIRCT may benefit from having graph iterators, and most/all of these will need an implementation exactly as what is given in InstanceGraphBase. My immediate usecase is to provide a graph for Ibis classes/containers, but things like handshake and FSM are other obvious users of this. The interface is named `InstanceGraphInterface`, but in practice naming it `CIRCTModuleInterface` or something similar would probably be more apt, seeing as the `InstanceGraph` is just a generic view of module-like things in CIRCT (which in turn is more generic than HWModule like) - LMKWYT. Implementing this change also performs some housekeeping in ensuring that all ops who inherit from the `HWModuleLike/HWInstanceLike` interfaces actually implements the name-related parts of the interface.

darthscsi

Fixing getReferencedModule is important, getInstance is a bit weird. Other than that, lgtm. Feel free to ignore my other PR.

darthscsi · 2023-08-14T14:41:21Z

include/circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h


 /// This graph tracks modules and where they are instantiated. This is intended
 /// to be used as a cached analysis on FIRRTL circuits.  This class can be used
 /// to walk the modules efficiently in a bottom-up or top-down order.
 ///
 /// To use this class, retrieve a cached copy from the analysis manager:
 ///   auto &instanceGraph = getAnalysis<InstanceGraph>(getOperation());
-class InstanceGraph : public hw::InstanceGraphBase {


I'm not completely convinced we need per-dialect versions of the InstanceGraph, but that isn't blocking for this.

darthscsi · 2023-08-14T14:44:54Z

include/circt/Support/InstanceGraph.h

+  auto getInstance() {
+    if constexpr (std::is_same<TTarget, InstanceOpInterface>::value)
+      return instance;
+    return dyn_cast_or_null<TTarget>(instance.getOperation());


dyn_cast should be sufficient. instance should always be non-null.

why is instance dyn_cast_or_null and module (below) is cast?

darthscsi · 2023-08-14T14:45:49Z

include/circt/Support/InstanceGraph.h

@@ -51,7 +55,12 @@ class InstanceRecord
    : public llvm::ilist_node_with_parent<InstanceRecord, InstanceGraphNode> {
 public:
  /// Get the instance-like op that this is tracking.
-  HWInstanceLike getInstance() const { return instance; }
+  template <typename TTarget = InstanceOpInterface>


I was pondering this functionality too. An alternate would be an explicitly specialized implementation for InstanceOpInterface

I also considered simply overloading the function in the per-dialect versions, but that gets a bit messy (though would eliminate the need for the op interfaces).

darthscsi · 2023-08-14T14:46:29Z

include/circt/Support/InstanceGraph.h

@@ -96,7 +105,12 @@ class InstanceGraphNode : public llvm::ilist_node<InstanceGraphNode> {
  InstanceGraphNode() : module(nullptr) {}

  /// Get the module that this node is tracking.
-  HWModuleLike getModule() const { return module; }
+  template <typename TTarget = ModuleOpInterface>


Same here, an explicitly specialized version might be clearer.

darthscsi · 2023-08-14T14:48:48Z

include/circt/Support/InstanceGraphInterface.td

+  let cppNamespace = "::circt::igraph";
+
+  let methods = [
+    InterfaceMethod<"Get the name of the instance",


Must all instances have names? This doesn't seem obviously true for all dialects. It seems primarily a requirement for pretty graph printing.

darthscsi · 2023-08-14T14:49:27Z

include/circt/Support/InstanceGraphInterface.td

+    "::mlir::StringAttr", "getInstanceNameAttr", (ins)>,
+
+    InterfaceMethod<"Get the name of the instantiated module",
+    "::llvm::StringRef", "getReferencedModuleName", (ins),


Should we instead be returning a symbolRefAttr?

darthscsi · 2023-08-14T14:50:28Z

include/circt/Support/InstanceGraphInterface.td

+    /*methodBody=*/[{}],
+    /*defaultImplementation=*/[{ return $_op.getModuleNameAttr().getAttr(); }]>,
+
+    InterfaceMethod<"Get the referenced module",


I don't think we should have this operation without taking a symbol table. Code which uses this without a symbol table tends to have n^2 behavior.

Not using a symbol table also has significant foot-guns related to passes and threading. I've added a note.

darthscsi · 2023-08-14T16:50:43Z

@mortbopet I hope you'll forgive my minor changes and committing this when it's done with CI. I want to update my other work in progress based on this PR.

mortbopet · 2023-08-14T17:20:13Z

@darthscsi no problem!

mortbopet requested a review from teqdruid August 13, 2023 16:02

mortbopet requested review from darthscsi, dtzSiFive, seldridge, fabianschuiki and maerhart as code owners August 13, 2023 16:02

mortbopet force-pushed the dev/mpetersen/generic_instancegraph branch from 332e556 to 22e0f4d Compare August 14, 2023 06:25

Morten Borup Petersen and others added 2 commits August 14, 2023 09:18

tidy

f2aa070

mortbopet force-pushed the dev/mpetersen/generic_instancegraph branch from 22e0f4d to f2aa070 Compare August 14, 2023 09:18

darthscsi approved these changes Aug 14, 2023

View reviewed changes

Hyjacking Morten's PR with some minor changes.

cea4f96

darthscsi requested review from Dinistro and lucas-rami as code owners August 14, 2023 16:38

darthscsi merged commit c366ef0 into main Aug 14, 2023
5 checks passed

darthscsi mentioned this pull request Aug 14, 2023

[NFC] Add an instantiable op interface. #5758

Closed

darthscsi deleted the dev/mpetersen/generic_instancegraph branch April 11, 2024 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HW/Support] Introduce InstanceGraph interface #5833

[HW/Support] Introduce InstanceGraph interface #5833

mortbopet commented Aug 13, 2023

teqdruid commented Aug 13, 2023

mortbopet commented Aug 14, 2023

darthscsi left a comment

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi Aug 14, 2023

darthscsi commented Aug 14, 2023

mortbopet commented Aug 14, 2023

[HW/Support] Introduce InstanceGraph interface #5833

[HW/Support] Introduce InstanceGraph interface #5833

Conversation

mortbopet commented Aug 13, 2023

teqdruid commented Aug 13, 2023

mortbopet commented Aug 14, 2023

darthscsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darthscsi commented Aug 14, 2023

mortbopet commented Aug 14, 2023