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

Add niosv #7

Open
wants to merge 22 commits into
base: development
Choose a base branch
from
Open

Add niosv #7

wants to merge 22 commits into from

Conversation

whitepau
Copy link
Owner

Adding a New Sample(s) - Nios V

Description

This code sample demonstrates how to use an FPGA IP produced with the Intel® oneAPI DPC++/C++ Compiler with a NIOS V soft CPU in an IP Authoring workflow.

This is the first pass at this code sample.

@yuguen-intel can you please review

  • readme (for understandability/process)
  • sample.json
  • kernel at kernels/simple_dma/

@jarrodblackburn can you please review

  • readme (for correctness)
  • kernel at kernels/simple_dma/
  • Intel® Quartus® Prime and Platform Designer stuff

I'll get Khai Liang to have a look after you, or if you don't have time

Checklist

Administrative

  • Review sample design with the appropriate Domain Expert:
  • If you have any new dependencies/binaries, inform the oneAPI Code Samples Project Manager

Code Development

Security and Legal

  • OSPDT Approval (see Project Manager for assistance)
  • Compile using the following compiler flags and fix any warnings, the falgs are: "/Wall -Wformat-security -Werror=format-security"
  • Bandit Scans (Python only)
  • Virus scan

Review

  • Review DPC++ code with Paul Peterseon. (GitHub User: pmpeter1)
  • Review readme with Tom Lenth(@tomlenth) and/or Project Manager
  • Tested using Dev Cloud when applicable

@whitepau whitepau requested a review from yuguen-intel August 16, 2023 11:53
Comment on lines 60 to 64
for (unsigned int i = 0; i < (length / 4);
i++) // will be testing with Nios V where an int is 4 bytes
{
destination[i] = source[i];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (unsigned int i = 0; i < (length / 4);
i++) // will be testing with Nios V where an int is 4 bytes
{
destination[i] = source[i];
}
// will be testing with Nios V where an int is 4 bytes
for (unsigned int i = 0; i < (length / 4); i++) {
destination[i] = source[i];
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this comment meant for the users or to remind the developer to test something?

Copy link
Owner Author

Choose a reason for hiding this comment

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

for users; to explain why we divide legnth by 4. do you think memcpy would make more sense here? @jarrodblackburn can you use memcpy on the Nios® V softcore processor?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah understood, then I think it should be rewritten as follows:

Suggested change
for (unsigned int i = 0; i < (length / 4);
i++) // will be testing with Nios V where an int is 4 bytes
{
destination[i] = source[i];
}
// will be testing with Nios V where an int is 4 bytes
constexpr kSize = length / 4
for (unsigned int i = 0; i < kSize; i++) {
destination[i] = source[i];
}

Copy link
Collaborator

@jarrodblackburn jarrodblackburn Aug 17, 2023

Choose a reason for hiding this comment

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

memcpy is present for Nios V but this is the kernel code so not it's applicable here. This change is fine given the DMA is only moving integers. The division was in the original code since I tend to keep everything expressed in bytes (unaligned DMAs get confusing if you deal with a mix of words and bytes).

Copy link
Owner Author

Choose a reason for hiding this comment

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

it shouldn't be a constexpr anyway because length is a runtime parameter. I'll write it like this:

  void operator()() const {
    // This loop does not handle partial accesses (less than 4 bytes) at the
    // start and end of the source/destination so ensure they are at least
    // 4-byte aligned
    for (unsigned int i = 0; i < (length_bytes / 4); i++) {
      destination[i] = source[i];
    }
  }

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.

3 participants