-
Notifications
You must be signed in to change notification settings - Fork 0
First commit for data_bundle tutorial #6
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| //////////////////////////////////////////////////////////////////////////////// | ||
| template <typename T, int n> | ||
| struct DataBundle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put DataBundle in a header file in the include directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I want the idea to be that the memory utilities work with any bundle-esque class, where DataBundle is one example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the plan to make the memory utilities generic, but I still think providing this bundle class in a separate header would be useful.
DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/data_bundle/src/data_bundle.cpp
Outdated
Show resolved
Hide resolved
| class USMProducerID; | ||
| class USMConsumerID; | ||
|
|
||
| event SubmitBufferProducer(queue &q, buffer<int, 1> &in_buf, size_t count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also encapsulate these producers/consumers into a shared header? Don't you already have this in the FakeIOPipes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to avoid having anything about SYCL queues in the shared header files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that's an important goal? I think if it's generically useful, which this clearly is, we should try to put it into the include directory. Maybe we isolate it into a special sub-directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see them as orthogonal, but I understand the confusion because of what I named this project.
I think we should have a generic "bundle" class that has operator[] semantics, but that didn't end up being my goal here (despite the name, sorry).
The goal here is to make the MemoryToPipe and PipeToMemory, and don't think these methods should depend on a specific bundle type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. Early monday. My response makes no sense.
I have no problem throwing the actual kernels into the file, but I kind of like the user having to wrap all of our utilities in kernels. That way they know what they are submitting.
| } | ||
|
|
||
| template<typename Pipe, typename PtrT> | ||
| void StreamToPipe(PtrT out_ptr, size_t count_div_elements_per_cycle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be called PipeToMemory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya. I messed up. Not too worried about names right now.
whitepau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readme doesn't contain any info about using data_bundle to pass data in parallel through a pipe; I think that deserves a shout-out.
|
|
||
|
|
||
| ## Purpose | ||
| This tutorial demonstrates how a kernel in a DPC++ FPGA program transfers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This tutorial demonstrates how a kernel in a DPC++ FPGA program transfers | |
| This tutorial demonstrates how a kernel in a DPC++ FPGA program streams |
I would like to lean on the idea that pipes are streams. particularly with an eye to IP Authoring.
No description provided.