Skip to content

[SYCL][FPGA][NFC] Minor update to fpga_lsu header templates #2375

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 1, 2020
Merged

[SYCL][FPGA][NFC] Minor update to fpga_lsu header templates #2375

merged 3 commits into from
Sep 1, 2020

Conversation

aditikum
Copy link
Contributor

Updating fpga_lsu.hpp templates to limit parameter scope and prevent conflicts with any defines.

@aditikum
[SYCL][FPGA] Minor update to fpga_lsu header templates
7553285 
Updating fpga_lsu.hpp templates to limit parameter scope and prevent conflicts when any defines that may use B or N.
@aditikum aditikum requested a review from a team as a code owner August 26, 2020 15:27
@aditikum aditikum requested a review from s-kanaev August 26, 2020 15:27
@s-kanaev
Copy link
Contributor

@aditikum please provide a more verbose description of the patch.

@aditikum
Copy link
Contributor Author

@s-kanaev Sure, the fpga_lsu.hpp file used the template parameter "N" originally. Users can have #define N as a parameter in the host & kernel code and this causes a conflict. To resolve this conflict, I'm changing the notation so all template parameters have an "_" before them. This should move it out of the users namespace. I've done this for both the "int N" and the "bool B" used in the lsu type templates.

@aditikum aditikum changed the title [SYCL][FPGA] Minor update to fpga_lsu header templates [SYCL][FPGA][NFC] Minor update to fpga_lsu header templates Aug 26, 2020
@aditikum
Updating fpga headers scope
b6a2af8 
User's code could cause conflict for headers local variables, internal types. and function parameters, adding the prefix "_"
s-kanaev
s-kanaev previously approved these changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

The changes look good.

@keryell
Copy link
Contributor

keryell commented Aug 27, 2020

Users can have #define N as a parameter in the host & kernel code and this causes a conflict.

Crazy world...
This doesn't prevent to teach also your users to use constexpr variables instead of macros. :-)

@keryell keryell mentioned this pull request Aug 27, 2020
Open
@aditikum
Update fpga_lsu.hpp GetValue calls
fe56227 
Used the locally scope GetValue as per the previous commit.
@aditikum
Copy link
Contributor Author

Updated pull request with a commit to update the remaining GetValue to the locally scoped GetValue. Should pass the checks now.

@aditikum aditikum requested a review from s-kanaev August 28, 2020 19:33
@aditikum
Copy link
Contributor Author

@s-kanaev The files are updated now!

s-kanaev
s-kanaev reviewed Aug 31, 2020
View reviewed changes
s-kanaev
s-kanaev approved these changes Aug 31, 2020
View reviewed changes
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

Approve for testing to start.

@romanovvlad romanovvlad merged commit 12bf4aa into intel:sycl Sep 1, 2020
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@callumfare
Merge pull request #2375 from lukaszstolarczuk/umf-bump-for-pulldown
bb67e5c 
[common] Bump UMF version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrSidims MrSidims MrSidims left review comments

@s-kanaev s-kanaev s-kanaev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aditikum @s-kanaev @keryell @MrSidims @romanovvlad