Merged
Conversation
Signed-off-by: tyoungsc <tanner.young-schultz@intel.com>
Signed-off-by: tyoungsc <tanner.young-schultz@intel.com>
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
tyoungsc
changed the title
Slight code cleanup
mdbtucker
reviewed
May 14, 2021
Collaborator
mdbtucker
left a comment
mdbtucker
left a comment
There was a problem hiding this comment.
Done reviewing all non-source (.hpp .cpp) files
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/third-party-programs.txt
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/CMakeLists.txt
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/CMakeLists.txt
Outdated
Show resolved
Hide resolved
CMake update Removed line from samples.json that was not necessary Deleted unused files
mdbtucker
reviewed
May 21, 2021
(to avoid runtime issue)
mdbtucker
reviewed
May 28, 2021
Collaborator
mdbtucker
left a comment
mdbtucker
left a comment
There was a problem hiding this comment.
Please confirm you've run clang-format on all code.
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/Misc.hpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/Misc.hpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/merge_sort.cpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/merge_sort.cpp
Outdated
Show resolved
Hide resolved
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
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/README.md
Outdated
Show resolved
Hide resolved
Improved Merge kernel for case where SORT_WIDTH=1 with shannonization
Variable renaming
Comments
Grammar
mdbtucker
reviewed
Jun 24, 2021
Collaborator
mdbtucker
left a comment
mdbtucker
left a comment
There was a problem hiding this comment.
Not many comments, I don't know if the code is in really good shape, or I'm feeling lenient, or I'm just lazy :-).
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/static_math.hpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/ReferenceDesigns/merge_sort/src/static_math.hpp
Outdated
Show resolved
Hide resolved
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
Comments
mdbtucker
approved these changes
Jun 24, 2021
Collaborator
mdbtucker
left a comment
mdbtucker
left a comment
There was a problem hiding this comment.
Looks good, you can go ahead and create a PR to the main repo.
tyoungsc
added a commit
that referenced
this pull request
Jul 6, 2021
…torials. (oneapi-src#553) * First commit for merge sort design Signed-off-by: tyoungsc <tanner.young-schultz@intel.com> * Cleaned up MergeSort.hpp a bit using defines Signed-off-by: tyoungsc <tanner.young-schultz@intel.com> * 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: tyoungsc tanner.young-schultz@intel.com
Description
Creating new FPGA reference design; merge sort (NOTE FROM REVIEWER: please add more detail here for the PR to the main repo)
External Dependencies
None
Type of change
How Has This Been Tested?
No method to test this with our infra yet, so I tested all 3 flows manually.