Skip to content

[SYCL] Remove arbitrary upper bound for attribute argument #3134

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 11 commits into from
Feb 17, 2021
Merged

[SYCL] Remove arbitrary upper bound for attribute argument #3134

merged 11 commits into from
Feb 17, 2021

Conversation

elizabethandrews
Copy link
Contributor

@elizabethandrews elizabethandrews commented Feb 2, 2021 •
edited
Loading

Remove arbitrary upper bounds for the following attributes -

SYCLIntelSchedulerTargetFmaxMhz
SYCLIntelLoopFuse
IntelFPGAPrivateCopies
IntelFPGABankBits
IntelFPGABankWidth
IntelFPGANumBanks
IntelFPGAMaxReplicates

Closes #2911

@elizabethandrews
[SYCL] Remove arbitrary range Part 1
845ff48 
Remove arbitrary range for SYCLIntelSchedulerTargetFmaxMhz and
SYCLIntelLoopFuse attributes.

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Rename function to be more generic
224b7eb 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Remove arb range part 2
9ba170f 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Copy link
Contributor Author

elizabethandrews commented Feb 2, 2021 •
edited
Loading

@AaronBallman could you take a look at this and let me know your thoughts. I renamed the IntelSYCL functions to be more generic so we can reuse it for other FPGA attributes as well. I think the function can be refactored but I thought I would upload this now just to get any feedback you may have before I work on the remaining attributes.

AaronBallman
AaronBallman reviewed Feb 2, 2021
View reviewed changes
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

I think the rename is problematic due to how Intel-specific the logic of the functions is. I think we should at least keep Intel in the name so that it's clear this is highly specific to our needs. Ultimately, I think we should be very wary of any function that needs to look at the parsed attribute kind. That's usually a code smell that the function isn't particularly generic and its exposure should be limited as much as possible.

@AaronBallman AaronBallman mentioned this pull request Feb 2, 2021
Closed
@elizabethandrews elizabethandrews changed the title [Do not merge][SYCL] Remove arbitrary range part 2 [Do not merge][Do not review][SYCL] Remove arbitrary range part 2 Feb 12, 2021
@elizabethandrews
Fix tests after merge and revert to less generic function names
7f8e23b 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Remove dead code
22753fe 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Implement review comments
6e4e924 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
AaronBallman
AaronBallman reviewed Feb 12, 2021
View reviewed changes
@elizabethandrews
Remove arbitrary range for remaining attributes
65e683c 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews changed the title [Do not merge][Do not review][SYCL] Remove arbitrary range part 2 [SYCL] Remove arbitrary upper bound for attribute argument Feb 12, 2021
@elizabethandrews elizabethandrews marked this pull request as ready for review February 12, 2021 14:53
elizabethandrews
elizabethandrews commented Feb 12, 2021
View reviewed changes
@elizabethandrews
Windows fail fix and implement review comments
d4e9983 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Fix MSVC build fail
68570c2 
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
smanna12
smanna12 reviewed Feb 12, 2021
View reviewed changes
smanna12
smanna12 reviewed Feb 12, 2021
View reviewed changes
smanna12
smanna12 reviewed Feb 15, 2021
View reviewed changes
AaronBallman
AaronBallman approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM!

premanandrao
premanandrao reviewed Feb 16, 2021
View reviewed changes
@elizabethandrews
Copy link
Contributor Author

Thanks for the reviews everyone!

@bader bader merged commit 489df6b into intel:sycl Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronBallman AaronBallman AaronBallman approved these changes

@premanandrao premanandrao premanandrao approved these changes

@smanna12 smanna12 smanna12 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.

[SYCL] Investigate arbitrary attribute bounds for FPGA attributes
5 participants
@elizabethandrews @AaronBallman @premanandrao @smanna12 @bader