cornell-zhang / heterocl

HeteroCL: A Multi-Paradigm Programming Infrastructure for Software-Defined Heterogeneous Computing
https://cornell-zhang.github.io/heterocl/
Apache License 2.0
322 stars 92 forks source link

[API] HLS Report Enhancement #337

Closed yn224 closed 3 years ago

yn224 commented 3 years ago

This PR attempts to improve upon the HLS report interface display, specifically targeting the latency information it returns.

Proposed interface:

...
report = f.report()
report.display( loops, levels, cols )

Here, loops represent an array of loop names, levels represent the maximum loop nest levels the report should print, and cols represent an array of latency category names.

An example output

+-----------+--------------+---------------+---------------+-------------------------+-------------------------+---------------+------------------+
|           |   Trip Count |   Min Latency |   Max Latency |   Min Iteration Latency |   Max Iteration Latency |   Pipeline II |   Pipeline Depth |
|-----------+--------------+---------------+---------------+-------------------------+-------------------------+---------------+------------------|
| A_x       |          410 |     104390920 |     105399520 |                  254612 |                  257072 |           N/A |              N/A |
| + A_y     |          615 |        254610 |        257070 |                     414 |                     418 |           N/A |              N/A |
| ++ A_ra0  |            5 |           410 |           410 |                      82 |                      82 |           N/A |              N/A |
| +++ A_ra1 |            5 |            80 |            80 |                      16 |                      16 |           N/A |              N/A |
| Y_x2      |          410 |      38831920 |      39840520 |                   94712 |                   97172 |           N/A |              N/A |
| + Y_y1    |          615 |         94710 |         97170 |                     154 |                     158 |           N/A |              N/A |
| ++ Y_ra4  |            3 |           150 |           150 |                      50 |                      50 |           N/A |              N/A |
| +++ Y_ra5 |            3 |            48 |            48 |                      16 |                      16 |           N/A |              N/A |
| X_x4      |          410 |      38831920 |      39840520 |                   94712 |                   97172 |           N/A |              N/A |
| + X_y2    |          615 |         94710 |         97170 |                     154 |                     158 |           N/A |              N/A |
| ++ X_ra2  |            3 |           150 |           150 |                      50 |                      50 |           N/A |              N/A |
| +++ X_ra3 |            3 |            48 |            48 |                      16 |                      16 |           N/A |              N/A |
| R_x6      |          410 |       6052420 |       6052420 |                   14762 |                   14762 |           N/A |              N/A |
| + R_y3    |          615 |         14760 |         14760 |                      24 |                      24 |           N/A |              N/A |
+-----------+--------------+---------------+---------------+-------------------------+-------------------------+---------------+------------------+
* Units in clock cycles

The current report interface only includes information about resource usage of the HLS simulation. In order to get more information regarding the HLS report (such as latency values), one has to traverse down the chain of directories created by the simulation, which can be cumbersome. However, by adding this new class rptdisp in this PR, we can also display latency information in addition to the default report that gets generated currently.

Possible limitations The proposed report interface enhancement does not support programs without any loops. Also, querying via regular expressions is unavailable as of now.

hecmay commented 3 years ago

It seems that the CI container is container circleci/python:3.6. And you can change it to 3.7 to fix the error.

seanlatias commented 3 years ago

It seems that the CI container is container circleci/python:3.6. And you can change it to 3.7 to fix the error.

I'm confused. Why do we need to change CI env? It seems the error is because the test is terminated.

seanlatias commented 3 years ago

Or more precisely, the test is canceled.

seanlatias commented 3 years ago

Make sure you merge or rebase the upstream. The CI Python env version is changed here. If the CI env needs to be changed, please file a PR instead.

https://github.com/cornell-zhang/heterocl/pull/323

hecmay commented 3 years ago

Or more precisely, the test is canceled.

The previous CI test failed with an error complaining that python 3.7 is required by not found.

I was trying to rerun the CI to pinpoint why. But I suddenly found the issue. So I cancelled it.

seanlatias commented 3 years ago

@yn224 can you change the comment format according to here?

seanlatias commented 3 years ago

Please update this file to include required packages. Also please do the following two things:

  1. Add unit tests.
  2. Extend your current PR texts to include: a) The expected outputs for your API and b) The main purposes/changes for the files you changed.
yn224 commented 3 years ago

@seanlatias I'm not too sure about these TVM errors that are getting raised

seanlatias commented 3 years ago

We don't have vivado_hls on CI. That's why I suggested you directly test on an existing report file instead of generating one.

seanlatias commented 3 years ago

@yn224 Please let me know if you're ready for review.

yn224 commented 3 years ago

@seanlatias I think it can be reviewed now.

yn224 commented 3 years ago

This is just the first round. Please fix these first and I'll do a second round.

@seanlatias I believe all of your comments are resolved. Further idea I have about testing includes querying with invalid column names and loop names. But they currently seem to raise IndexError, which I still need to investigate more.

seanlatias commented 3 years ago

Great, I'll take a look.