Skip to content

Max parameter length fetched from device #3032

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
9prady9 merged 2 commits into from
Nov 4, 2020
Merged

Max parameter length fetched from device #3032

9prady9 merged 2 commits into from
Nov 4, 2020

Conversation

@willyborn
Copy link
Contributor

@willyborn willyborn commented Oct 26, 2020 •
edited
Loading

On some devices, the hard-coded maxima for the parameter length did not correspond. This is solved by asking it to the device and using the returned value.

Description

  • Fixes errors and crashes on iGPUs other than intel, which was also hard-coded.
  • Noticed impact is a speed improvement for some devices.
  • The hard-coded values are now removed, so this is not tested for other devices.
  • If desired, this can be back-ported since the change is very limited.

Changes to Users

  • No changes, except on improved stability.

Checklist

  • Rebased on latest master
  • Code compiles
  • Tests pass

@umar456
Copy link
Member

umar456 commented Oct 26, 2020

This is interesting. I didn't know this parameter existed! I will test it on other devices and get back to you. Please, update the formatting of the code added and if possible please rebase your changes on top of master and removing the merge commit.

Would also be nice if you could make the gitignore commit message more descriptive.

Thanks

@willyborn willyborn changed the title Parameter length overflow during opencl program build Max parameter length fetched from device Oct 27, 2020
@willyborn willyborn force-pushed the ParameterLengthOverflow branch 4 times, most recently from 682fa65 to 08d780b Compare October 27, 2020 19:12
@9prady9
Copy link
Member

9prady9 commented Oct 29, 2020

This is interesting. I didn't know this parameter existed! I will test it on other devices and get back to you. Please, update the formatting of the code added and if possible please rebase your changes on top of master and removing the merge commit.

Would also be nice if you could make the gitignore commit message more descriptive.

Thanks

Yes, for whatever reason, this isn't listed on online documentation of clGetDeviceInfo. However, it is present in the PDF specification sheets of versions 1.2, 2.2 and 3.0. So, it is not an extension or recent addition.

9prady9
9prady9 reviewed Oct 29, 2020
View reviewed changes
Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

Looks good.

src/backend/opencl/Array.cpp Outdated
int heightCheckLimit =
isIntel && getDeviceType() == CL_DEVICE_TYPE_GPU ? 3 : 6;
isIntel && device.getInfo<CL_DEVICE_TYPE>() == CL_DEVICE_TYPE_GPU ? 3
: 6;
Copy link
Member

@9prady9 9prady9 Oct 29, 2020

Choose a reason for hiding this comment

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

This and the earlier line as essentially identical as device returned

@willyborn
Max parameter length is now fetched from device.
4e2b5ff 
Values for opencl parameter maximum length were hardcoded.  The maximum is now requested at the device, so that the correct value for all devices is used.
@willyborn willyborn force-pushed the ParameterLengthOverflow branch from a219ab9 to 4e2b5ff Compare November 1, 2020 21:15
9prady9
9prady9 requested changes Nov 2, 2020
View reviewed changes
Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

Looks good and works for NVIDIA and AMD(Kaveri APU) for all tests except one which failed on both devices.

Random number quality test is failing for both my GTX 1060 with CUDA 11.1 and AMD API Kaveri APU. Not sure if it is a problem from PR or master yet. @umar456 Can you please confirm if this is a known issue or new one.

One minor change, can you please remove the isNvidia and isAmd variables that aren't used anymore. They appear in warnings now, they are declared in the lines 303, 305.

Thank you.

@9prady9 9prady9 merged commit 0493478 into arrayfire:master Nov 4, 2020
9prady9 pushed a commit to 9prady9/arrayfire that referenced this pull request Aug 2, 2021
@willyborn @9prady9
Max parameter length fetched from device (arrayfire#3032)
1b5380c 
* Max parameter length is now fetched from device.

Values for opencl parameter maximum length were hardcoded.  The maximum is now requested at the device, so that the correct value for all devices is used.

* Removed isAmd & isNvidia, since they are no longer used.

(cherry picked from commit 0493478)
syurkevi pushed a commit that referenced this pull request Dec 28, 2021
@willyborn @syurkevi
Max parameter length fetched from device (#3032)
239ce3d 
* Max parameter length is now fetched from device.

Values for opencl parameter maximum length were hardcoded.  The maximum is now requested at the device, so that the correct value for all devices is used.

* Removed isAmd & isNvidia, since they are no longer used.

(cherry picked from commit 0493478)
@willyborn willyborn deleted the ParameterLengthOverflow branch September 29, 2022 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@umar456 umar456 umar456 approved these changes

@9prady9 9prady9 9prady9 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.

3 participants

@willyborn @umar456 @9prady9