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

Support for Patmos platform #2383

Merged
merged 29 commits into from
Oct 24, 2024
Merged

Support for Patmos platform #2383

merged 29 commits into from
Oct 24, 2024

Conversation

EhsanKhodadad
Copy link
Collaborator

No description provided.

@lhstrh
Copy link
Member

lhstrh commented Aug 2, 2024

@EhsanKhodadad, I enabled CI for you, but it looks like there are still some test failures. Please address those first.

@EhsanKhodadad
Copy link
Collaborator Author

@EhsanKhodadad, I enabled CI for you, but it looks like there are still some test failures. Please address those first.

Thank you for enabling CI. The tester has not found a header file in the t-crest repository, because t-crest hasn't cloned on the tester's machine.

@lhstrh
Copy link
Member

lhstrh commented Aug 3, 2024

Got it. Can you fix it in the workflow?

@EhsanKhodadad
Copy link
Collaborator Author

Got it. Can you fix it in the workflow?

Sure

@lhstrh
Copy link
Member

lhstrh commented Aug 22, 2024

Btw, I noticed tests in your fork were failing. Here's a fix for that: #2402

@EhsanKhodadad
Copy link
Collaborator Author

EhsanKhodadad commented Oct 4, 2024

I have two questions regarding this PR:

  1. Since I haven't implemented multi-thread functions and variables like mutex and thread in lf-patmos-support yet, can I bypass the tests in the concurrent folder temporarily? If so how?
  2. In the regular tests, all LF apps, including my HelloPatmos.lf app is tested at the beginning of the test procedure, whereas the Patmos library, including Patmos-clang, still needs to be installed. Can I postpone their execution after the Patmos installation? (Something similar to Patmos-tests, where benchmarks are run after Patmos installation.) These tests are written in .github/workflows/c-test.yml file, while the Patmos installation is invoked by c-patmos-test.yml.

@EhsanKhodadad
Copy link
Collaborator Author

EhsanKhodadad commented Oct 11, 2024

I have two questions regarding this PR:

  1. Since I haven't implemented multi-thread functions and variables like mutex and thread in lf-patmos-support yet, can I bypass the tests in the concurrent folder temporarily? If so how?
  2. In the regular tests, all LF apps, including my HelloPatmos.lf app is tested at the beginning of the test procedure, whereas the Patmos library, including Patmos-clang, still needs to be installed. Can I postpone their execution after the Patmos installation? (Something similar to Patmos-tests, where benchmarks are run after Patmos installation.) These tests are written in .github/workflows/c-test.yml file, while the Patmos installation is invoked by c-patmos-test.yml.

@lhstrh @erlingrj @lsk567 Could you answer these questions, please? Thanks.

@erlingrj
Copy link
Collaborator

erlingrj commented Oct 11, 2024

I have two questions regarding this PR:

  1. Since I haven't implemented multi-thread functions and variables like mutex and thread in lf-patmos-support yet, can I bypass the tests in the concurrent folder temporarily? If so how?

Check out your file CPatmosTests.java, here you explicitly run these tests with buildPatmosConcurrent remove this function and then change the CI so it doesn't try to run the gradle task associated with ut.

  1. In the regular tests, all LF apps, including my HelloPatmos.lf app is tested at the beginning of the test procedure, whereas the Patmos library, including Patmos-clang, still needs to be installed. Can I postpone their execution after the Patmos installation? (Something similar to Patmos-tests, where benchmarks are run after Patmos installation.) These tests are written in .github/workflows/c-test.yml file, while the Patmos installation is invoked by c-patmos-test.yml.

You should not run any Patmos-specific tests in C/regular-tests. They should be excluded from there and only be built in there special target. Do a search for ZEPYHR_UNTHREADED in the code-base to see the additions needed to disable a test category from the regular tests (and also from the CCpp tests)

@lhstrh lhstrh requested a review from erlingrj October 21, 2024 23:41
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This overall look good! Let me defer to @erlingrj for final approval.

@lhstrh lhstrh added this to the 0.9.0 milestone Oct 21, 2024
Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

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

Looks good, but we should not update reactor-cpp submodule in this PR

core/src/main/resources/lib/cpp/reactor-cpp Outdated Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Oct 24, 2024

@EhsanKhodadad, please make sure reactor-cpp points to dfdac2c.

@lhstrh lhstrh dismissed erlingrj’s stale review October 24, 2024 18:44

Submodule update has been undone.

Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

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

Looks good, lets merge :)

@erlingrj erlingrj added this pull request to the merge queue Oct 24, 2024
Merged via the queue into lf-lang:master with commit daaea1b Oct 24, 2024
24 checks passed
@schoeberl
Copy link
Collaborator

Finally, very good!

lhstrh added a commit to lf-lang/lf-lang.github.io that referenced this pull request Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants