Skip to content

Update README.md #1

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

Closed
wants to merge 13 commits into from
Closed

Update README.md #1

wants to merge 13 commits into from

Conversation

mkitez
Copy link
Contributor

@mkitez mkitez commented Jun 10, 2020

No description provided.

@samples-ci samples-ci self-requested a review June 10, 2020 06:53
samples-ci
samples-ci previously approved these changes Jun 10, 2020
@anjgola anjgola self-requested a review June 10, 2020 18:47
anjgola
anjgola previously approved these changes Jun 10, 2020
@mkitez mkitez dismissed stale reviews from anjgola and samples-ci via e242306 June 16, 2020 07:32
@@ -6,5 +6,74 @@
"languages": [{"cpp": {"properties": {"projectOptions": [{"projectType": "makefile"}]}}}],
"targetDevice": ["CPU", "GPU", "FPGA"],
"os": ["linux", "windows"],
"builder": ["ide", "make"]
"builder": ["ide", "make"],
"configurations": {
Copy link
Contributor

@xmnboy xmnboy Jun 16, 2020

Choose a reason for hiding this comment

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

Roman -- I think the name of the CI testing object should be more direct. The word configurations does not communicate to me that this contains CI tests. How about calling the object ciTests?

Suggested change
"configurations": {
"ciTests": {

Comment on lines 12 to 23
{
"id": "cpu_usm",
"build": [
"make all"
],
"run": [
"make run"
],
"clean": [
"make clean"
]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Roman -- I think my suggestion (below) is consistent with what you were alluding to, yesterday. Rather than being specific to the build, you might want to have a more general set of "phases" that direct the tests to be performed (mind you, I'm not a CI expert, so take my suggestions with a "grain of salt.").

Perhaps something like the following, which are overly complex to make a point, not to indicate that all tests would be like this (and where all array element "actions" are assumed to start from the root of the sample):

    "linux": [
      {
        "id": "cpu_usm",
        "setup": [ 
          [ "mkdir -p test/test && cd test/test && echo 'x=y' >test.txt", "/success-regex/", "/fail-regex/" ],
          [ "export TEST=test ; touch abc ; my-setup-script.sh || true", "/success-regex/", "/fail-regex/" ]
        ],
        "test": [
          [ "mkdir build && cd build && cmake ..", "/success-regex/", "/fail-regex/" ]
        ],
        "clean": [
          [ "make clean", "", "" ]
        ]
      }
    ],

Where the idea is that a sequence of commands that could be executed at the shell, with optional "success" and "fail" regex strings provided to spot a success or failure based on the output (if any) of the commands. I've made the above overly complex just to make the point that I don't think you want to assume a particular build tool, rather that you just want to know "what do I type to setup for a test" and "what do I type to test" and "what key phrases/output" do I expect to see on success/failure?

I would also assume that you would continue to check the return code from each "action" as part of the testing, assuming that a non-zero value is a pass and anything else is a fail...

Anyway, just a thought, any further than what I have suggested above you'd probably want to use something like YAML rather than JSON to use as the testing template.

Again, I emphasize that this is just an idea. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What goes into the test command line is completely up to the sample owner. If they want to validate output against a regex, they can do it as long as the command returns zero on success and non-zero otherwise. I don't think we can reliably tell in CI whether the sample is running well other than by checking the exit status.

I'm still not sure how to break down the test commands, by build/run/clean stages or by test phases as you suggested or have them as a single command at all. Or there can be arbitrary stages. For example some samples may omit the clean step, others may include more steps, e.g. pre-build but I'm worried it may complicate the things.

Copy link

@samples-ci samples-ci left a comment

Choose a reason for hiding this comment

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

Header

Test

Copy link

@samples-ci samples-ci left a comment

Choose a reason for hiding this comment

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

Change request

Copy link
Contributor

@anjgola anjgola left a comment

Choose a reason for hiding this comment

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

Is this good to merge?

@mkitez mkitez closed this Jun 30, 2020
@JoeOster JoeOster mentioned this pull request Apr 5, 2021
1 task
mdbtucker pushed a commit that referenced this pull request Jul 6, 2021
…torials. (#553)

* First commit for merge sort design

Signed-off-by: tyoungsc <[email protected]>

* Cleaned up MergeSort.hpp a bit using defines

Signed-off-by: tyoungsc <[email protected]>

* Formatting/comments

* Reduced area by connecting Partition unit (previously called Shuffle) to
only the first merge unit
Changed Shuffle to Partition
Updated pictures and README
Used existing pipe_array code (instead of using my own)
Code cleanup and comments

Tested in emulation and reports, doing a HW build now

* Fixed bug with no-USM BSPs in Produce kernel
Slight code cleanup

* README update after Mike's review
CMake update
Removed line from samples.json that was not necessary
Deleted unused files

* Changed Windows specific flag to use '/' instead of '-'

* CMake update

* Removed all pointer arithmetic for offseting to the inside of kernels
(to avoid runtime issue)

* Big change to example design: encorperate a new multi-element per cycle
merge unit, use a bitonic sorter on the input, rather than a simple
partition.

Updated README and pictures to fit the new design.

Addressed Mike's most recent review comments

* Code cleanup: changed some variable names and comments

* README update

* Picture update

* Comments

* Code cleanup and comments

* Small change to README

* Merged shannonization VCXPROJ files into a single one (as per all the
other tests).
Small update to source file to change II target for A10 and fix indenting

* Adding file I missed in previous commit

* Updated comment

* Grammar update

* Again

* Updating design output and README

* Simplified Produce kernel

* Allowed SORT_WIDTH to be 1 (1 element per cycle)
Improved Merge kernel for case where SORT_WIDTH=1 with shannonization

* Code cleanup
        Variable renaming
        Comments
        Grammar

* Comments, formatting, README grammar changes, and general cleanup :)

* Renamed all files to match google style
Renamed main file to main.cpp
Used impu namespace in unrolledloop, pipearray, and static_math
Renamed static_math.hpp to impu_math.hpp
Updated README, Windows VS files, and CMake with these changes

* README updates
Comments

* Changed filenames to use underscores between words

* Added 'pipe' namespace to impu namespace for pipe utilities

* Revert "Merge pull request #1 from tyoungsc/new_fpga_ref_design_merge_sort"

This reverts commit 202ff82, reversing
changes made to 72580ac.
jimmytwei pushed a commit that referenced this pull request Oct 30, 2024
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.

4 participants