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

Fix build for circt 2500cf570bbc6c6d45940466f9421e93d3a49132 #12

Merged
merged 3 commits into from
Nov 28, 2022

Conversation

tshadley
Copy link

This was in reference to #11

All tests pass except for cosim insertion_sort (hanging in mlir-cpu-runner):

"Timed Out Test: CIRCT-HLS cosim :: suites/Dynamatic/insertion_sort/tst_insertion_sort.c"

I also added a bash build script "build.sh" for Ubuntu 22.04.1 LTS

Let me know if this is helpful or if further steps/updates should be done. Also open to debugging insertion_sort.c further if you have any hints.

Best regards,
-Tedd

(still Timed Out Test: CIRCT-HLS cosim :: suites/Dynamatic/insertion_sort/tst_insertion_sort.c)

Add build.sh for bash build under Ubuntu 22.04.1 LTS
Copy link
Member

@mortbopet mortbopet left a comment

Choose a reason for hiding this comment

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

Thank you very very much for this! Regular bumps are important for this repo, so glad to have help with this.
wrt. the cosim test that is failing - no worries, it's probably due to some upstream logical changes. It's more important that this repo gets up and running.

To improve things even further, would you mind:

  • adding a circt_tag.txt file in the base of this repository which points to the latest CIRCT version that we're tracking (i,e. the commit hash of your bump).
  • modify README.md to describe this version as our "known good" version (at the place where (after this).
  • For a+ effort, modify our CI builds such that they will load this tag file and use that as the commit hash for the CIRCT checkout

cosim_test/lit.cfg.py Outdated Show resolved Hide resolved
include/circt-hls/Dialect/Cosim/CMakeLists.txt Outdated Show resolved Hide resolved
include/circt-hls/InitAllDialects.h Outdated Show resolved Hide resolved
tools/hls-opt/CMakeLists.txt Outdated Show resolved Hide resolved
tools/hls-opt/hls-opt.cpp Outdated Show resolved Hide resolved
Added "circt_tag.txt".

CIRCT tag file now used by CI build via juliangruber/read-file-action@v1

Modified README.md with CIRCT version and failure case.

Cleaned up unnecessary commenting.
@tshadley
Copy link
Author

Glad to help! I also appreciate your guiding me through the process.

I bumped CIRCT to 062eab31a67c8a8418b2ba9fb4f1814cdca08783 with no issues.

adding a circt_tag.txt file

Added this file.

modify README.md to describe this version as our "known good" version (at the place where (after this).

Done. I also added a line about build.sh, free free to omit if preferred (I find I prefer the bash flow to anything else at the moment).

For a+ effort, modify our CI builds such that they will load this tag file and use that as the commit hash for the CIRCT checkout

I used juliangruber/read-file-action@v1 to read the tag file into a variable and specified that as CIRCT ref. I tested with act (which runs github actions locally), but this might not be the same as a true github flow.

I cleaned up the unneccessary comments.

@tshadley
Copy link
Author

I originally ran into the apt-get 404 errors using the act local workflow simulator. These were fixed by calling
apt-get update --fix-missing

I assumed these were due to some eccentricities of act but I see you're getting the same error now using github workflow? This might be due to the docker image being slightly out of date now. In any case, fix-missing seems to solve it on my flow (78febfc)

@mortbopet mortbopet merged commit 5ea1f87 into circt-hls:main Nov 28, 2022
@tshadley tshadley deleted the merge_latest_circt branch November 28, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants