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.

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

DFCxx simulation #49

Merged
merged 23 commits into from
Sep 5, 2024
Merged

DFCxx simulation #49

merged 23 commits into from
Sep 5, 2024

Conversation

Muxianesty
Copy link
Collaborator

@Muxianesty Muxianesty commented Aug 16, 2024

This Pull Request introduces DFCxx simulation to Utopia HLS.

[!!!] NO REVIEWER SELECTED YET. THIS PULL REQUEST IS INCOMPLETE.
Pull Request ready for review.

@Muxianesty Muxianesty added the enhancement New feature or request label Aug 16, 2024
@Muxianesty Muxianesty self-assigned this Aug 16, 2024
@Muxianesty Muxianesty added the stage II For issues applicable to Stage II of the project label Aug 16, 2024
@Muxianesty Muxianesty marked this pull request as draft August 17, 2024 17:35
@Muxianesty Muxianesty marked this pull request as ready for review August 26, 2024 17:14
Copy link
Collaborator

@ssmolov ssmolov left a comment

Choose a reason for hiding this comment

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

Too many copy-paste, I am exhausted :((

README.md Outdated

* `-h,--help`: *optional* flag; used to print the help-message about other arguments.
* `--in <PATH>`: *optional* filesystem-path option; used to specify the input file for simulation data (default: `sim.txt`). Its format is presented in *DFCxx Simulation Input Format* section.
* `--out <PATH>`: *optional* filesystem-path option; used to specify the output VCD file (default: `sim_out.txt`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks strange that the output VCD file has sim_out.txt name. A "*.vcd" extension would be better.

README.md Outdated
@@ -268,6 +274,33 @@ Here is an example of a JSON configuration file, containing latencies for multip
}
```

### DFCxx Simulation Input Format
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, it would be better to have a concise README and describe auxiliary information in separate Markdown files. Let's move this chapter there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Trailing whitespace is detected here also.

README.md Outdated
@@ -268,6 +274,33 @@ Here is an example of a JSON configuration file, containing latencies for multip
}
```

### DFCxx Simulation Input Format

DFCxx kernels can be simulated to check that they describe computations as expected. The simulation doesn't include scheduling-related characteristics, thus DFCxx concepts like *offsets* are not supported, every computational node has to use **and** accept some value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

First sentence is ok. The second one is hardly understandable; could be better to have the specific section called "Supported DFCxx constructions" (or something like that) in this document.

README.md Outdated
DFCxx kernels can be simulated to check that they describe computations as expected. The simulation doesn't include scheduling-related characteristics, thus DFCxx concepts like *offsets* are not supported, every computational node has to use **and** accept some value.
The format to provide simulation input data is the following:

* input data is divided into blocks separated with a newline character (`\n`) - one block for each clock period
Copy link
Collaborator

Choose a reason for hiding this comment

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

"for each clock period" -> "at a simulation step"

README.md Outdated
The format to provide simulation input data is the following:

* input data is divided into blocks separated with a newline character (`\n`) - one block for each clock period
* every block has a number of lines, each of which has the name of some **input** stream/scalar value and its hex-value (these values do not have a type - it is assumed from the corresponding computational nodes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

"these values do not have a type - it is assumed from the corresponding computational nodes" -> "these values do not have an explicit type - they will be casted to the types of related computational nodes"

Comment on lines 173 to 176
meta.storage.addVariable(newVar);
meta.graph.addNode(newVar, OpType::NEQ, NodeData{});
meta.graph.addChannel(this, newVar, 0, false);
meta.graph.addChannel(&rhs, newVar, 1, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy-paste

Comment on lines 196 to 198
meta.storage.addVariable(newVar);
meta.graph.addNode(newVar, OpType::SHR, NodeData{.bitShift=bits});
meta.graph.addChannel(this, newVar, 0, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy-paste

src/options.h Outdated
}
}

std::string latConfigFile;
Copy link
Collaborator

Choose a reason for hiding this comment

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

latConfigFile -> latencyCfgFile

src/options.h Outdated
}
}

std::string latConfigFile;
DFLatencyConfig latConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

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

latConfig -> latencyCfg

src/options.h Outdated
Comment on lines 295 to 296
get(json, SIM_IN_JSON, inFilePath);
get(json, SIM_OUT_JSON, outFilePath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra spaces between args

Copy link
Collaborator Author

@Muxianesty Muxianesty left a comment

Choose a reason for hiding this comment

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

Fixes regarding comments above were made.

README.md Outdated

The list of arguments for `hls`-mode is presented below:
`hls` mode is used to translate the provided DFCxx kernel to different output formats. The list of arguments for `hls`-mode is presented below:
Copy link
Collaborator

Choose a reason for hiding this comment

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

hls mode is used to perform a High-Level Synthesis of digital hardware description from the input DFCxx kernel

README.md Outdated
@@ -232,43 +232,13 @@ Here is an example of an Utopia HLS CLI call:
umain hls --config ~/utopia-user/config.json --out-sv ~/outFile.sv --out-dfcir ~/outFile2.mlir -a
```

### JSON Configuration
`sim` mode is used to simulate the provided DFCxx kernel. The list of arguments for `sim`-mode is presented below:
Copy link
Collaborator

Choose a reason for hiding this comment

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

"provided DFCxx kernel" -> "input DFCxx kernel"

@@ -0,0 +1,35 @@
## JSON Configuration

Latency configuration for each computational operation (number of pipeline stages) used in a DFCxx kernel is provided via a JSON file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Latency configuration for each computational operation (number of pipeline stages)" -> "Latency configuration (in terms of pipeline stages) for each computational operation"


Latency configuration for each computational operation (number of pipeline stages) used in a DFCxx kernel is provided via a JSON file.

Currently each operation has two specifications: for integer values (`INT`) and floating point (`FLOAT`) values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Each operation has two specifications are based on the supported types of arguments: integer (INT) and floating point (FLOAT) respectively.

...or something like that


Currently each operation has two specifications: for integer values (`INT`) and floating point (`FLOAT`) values.

The list of all computational operations is provided below:
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the supported computational operations are listed below:

Comment on lines 268 to 269
const std::unordered_map<Node,
std::string> &idMap) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line-splitted type again

src/model/dfcxx/lib/dfcxx/vars/scalar.cpp Show resolved Hide resolved
src/model/dfcxx/lib/dfcxx/vars/scalar.cpp Show resolved Hide resolved
src/model/dfcxx/lib/dfcxx/vars/stream.cpp Show resolved Hide resolved
src/model/dfcxx/lib/dfcxx/vars/stream.cpp Show resolved Hide resolved
Copy link
Collaborator Author

@Muxianesty Muxianesty left a comment

Choose a reason for hiding this comment

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

Fixed according to comments above.

@@ -0,0 +1,35 @@
## JSON Configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

File was renamed, but the title remains the same -- is it ok?

@@ -0,0 +1,35 @@
## JSON Configuration

Latency configuration (in terms of pipeline stages) for each computational operation used in a DFCxx kernel is provided via a JSON file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no information about pipeline stages in this document, so the refinement looks like "hanging in the air" fact.

Also passive voice is used, it makes sentence complicated. It would be more clear to start like "Latency configuration is a JSON-based file describing characteristics of computational operations for the specific DFCxx kernel."

map[first], attr);
break;
}
default: assert(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will be helpful to print an informative message that shows unexpected 'node.type' value

Copy link
Collaborator Author

@Muxianesty Muxianesty left a comment

Choose a reason for hiding this comment

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

Fixed according to comments above.

Copy link
Collaborator

@ssmolov ssmolov left a comment

Choose a reason for hiding this comment

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

Ok, merged

@ssmolov ssmolov merged commit be9a08f into master Sep 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stage II For issues applicable to Stage II of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants