[Buffers] Subject Graph Implementation for MapBuf

[cosmiclat05]

Introduction

This Pull Request introduces SubjectGraph.cpp and SubjectGraph.h, which implement functionality to transform MLIR Operations into Subject Graph representations.

Key Components

1. SubjectGraphGenerator:

   • Loops over Operations of a FuncOp, and calls the corresponding Subject Graph constructor for each Operation.
   • After all of the Subject Graphs are created, SubjectGraphs are looped over one more time to populate their input/output vectors.

2. BaseSubjectGraph:

   • Parent class of hardware module-specific Subject Graph classes.
   • Each object of this type holds its own data structures for input/output modules.
   • Inputs/outputs are saved with their MLIR Result Number, which is important to get the correct input/output module.

3. Module Specific Derived Classes:

   • Based on hardware description characteristics, generates a path to read from the corresponding .blif file (size, data_type, addr_type, select_type etc.)
   • Blif file is read into a LogicNetwork object.
   • Then, based on the naming conventions used in hardware description, different signals are categorized accordingly.
   • For example, a fork consists of I/Os of type "ins" and "outs", with ready/valid/data signals. The number of inputs is based on its SIZE attribute.
   • An adder consists of "lhs", "rhs", "result".
   • Output nodes are renamed based on the unique name of each module. The renaming here follows a similar convention to how ABC would do it.

4. Merging all Subject Graphs:

   • After individual Subject Graphs are generated, they are merged into a single LogicNetwork object using connectInputNodes() and returnOutputNodes() functions.
   • A module can have multiple outputs, and multiple inputs. For example, 5th output channel of a fork might be connected to 2nd input of an adder. All input/output sets are categorized based on their corresponding MLIR Result Number.
   • Nodes are merged by using the functionality provided in BlifReader.cpp.

[murphe67]

Hey Osman! Thanks for the nice pull request!

I left a couple of superficial comments to begin with, but my main request right now is a little bit more vague- could you add a subfolder to docs for MapBuf and write up a little bit of documentation of what these subject graphs are used for/how they are used?

I think it would be super useful to keep this code accessible :)

For the code itself, it is very nice, I like the general structure + the polymorphism. However, I think your internal datastructures are totally crazy-

dynamatic/experimental/include/experimental/Support/SubjectGraph.h

Lines 76 to 97 in 626024f

	// Vectors and Maps to hold the input and output modules and SubjectGraphs.
	// (input/output)Modules and ToResNum is populated the first time the
	// SubjectGraph is created.
	std::vector<Operation *> inputModules;
	std::vector<Operation *> outputModules;
	DenseMap<Operation *, unsigned int> inputModuleToResNum;
	DenseMap<Operation *, unsigned int> outputModuleToResNum;
	// The populated maps store channel-specific information that connects
	// SubjectGraphs together. Each SubjectGraph has a position in the vector
	// which determines what type of data it handles (e.g., data, address, index).
	// Separately, a Result Number identifies which specific output channel of an
	// Operation to connect to, since an Operation may have multiple inputs and
	// outpus. The maps are essential for establishing the correct connections
	// between SubjectGraphs, while the vectors determine the data types being
	// passed through those connections.
	std::vector<BaseSubjectGraph *> inputSubjectGraphs;
	std::vector<BaseSubjectGraph *> outputSubjectGraphs;
	DenseMap<BaseSubjectGraph *, unsigned int> inputSubjectGraphToResNum;
	DenseMap<BaseSubjectGraph *, unsigned int> outputSubjectGraphToResNum;
	static inline DenseMap<Operation *, BaseSubjectGraph *> moduleMap;

Data structures these intense are not always a bad thing (sometimes we really do need them) but as they get more complex the need for documentation skyrockets.

There also seems to be a lot of duplication in the name generation/assign signals logic, but without comments it is difficult to know if it is needed or not.

Could you possibly close this pull request, and make another with less classes, so we can focus in on just the first 2 for discussion to begin with? I think we would get through it a little bit faster :)

Thanks again for the all the hard work!

[murphe67]

maybe combine these two in a static function?

[murphe67]

current node is a pointer passed by value. changing the value of the pointer doesn't have any impact outside of the function body, or am I wrong?

is this supposed to change the pointer passed to the function? should it delete the previous current node first, if so?

[murphe67]

really like this function but think its name is a bit unclear- to me the name kinda implies it sets io to true?

what do you think of convertIOToChannel ?

[murphe67]

maybe leave a comment on why there would be no defining op? should this be an error, or is this a known case?

[murphe67]

if you are inside of the MLIR infrastructure, maybe this should be done using the MLIR diagnostic library?

It looks to me this would be a factory function which returns a FailureOr, and could call op->emitError?

I'm not super experienced with this, the example I am drawing from is-

dynamatic/lib/Dialect/HW/InnerSymbolTable.cpp

Lines 44 to 47 in 261ba42

	FailureOr<InnerSymbolTable> InnerSymbolTable::get(Operation *op) {
	assert(op);
	if (!op->hasTrait<OpTrait::InnerSymbolTable>())
	return op->emitError("expected operation to have InnerSymbolTable trait");

[murphe67]

what do you think of the name of this function?
I'm not fully following the code, but it doesn't seem to "replace". You appear to have this interim subject graphs vector you use to build two final vectors, is that right?