-
Notifications
You must be signed in to change notification settings - Fork 2
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
DFCxx simulation #49
Conversation
…simplified; DFCXXSimulator class was added.
… example minor fixes.
…data file instead of a vector.
There was a problem hiding this 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`). |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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"
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-paste
meta.storage.addVariable(newVar); | ||
meta.graph.addNode(newVar, OpType::SHR, NodeData{.bitShift=bits}); | ||
meta.graph.addChannel(this, newVar, 0, false); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
get(json, SIM_IN_JSON, inFilePath); | ||
get(json, SIM_OUT_JSON, outFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra spaces between args
There was a problem hiding this 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: |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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"
docs/latency_config.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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"
docs/latency_config.md
Outdated
|
||
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. |
There was a problem hiding this comment.
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
docs/latency_config.md
Outdated
|
||
Currently each operation has two specifications: for integer values (`INT`) and floating point (`FLOAT`) values. | ||
|
||
The list of all computational operations is provided below: |
There was a problem hiding this comment.
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:
const std::unordered_map<Node, | ||
std::string> &idMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line-splitted type again
There was a problem hiding this 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.
docs/latency_config.md
Outdated
@@ -0,0 +1,35 @@ | |||
## JSON Configuration |
There was a problem hiding this comment.
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?
docs/latency_config.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, merged
This Pull Request introduces DFCxx simulation to Utopia HLS.
[!!!] NO REVIEWER SELECTED YET. THIS PULL REQUEST IS INCOMPLETE.Pull Request ready for review.