FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Max parameter length fetched from device by willyborn · Pull Request #3032 · arrayfire/arrayfire · GitHub

  1. arrayfire
  2. arrayfire

Repository navigation

Max parameter length fetched from device - #3032

Merged
9prady9 merged 2 commits into
arrayfire:masterfrom
willyborn:ParameterLengthOverflow
Nov 4, 2020
ConversationCommits (2)ChecksFiles changed
Merged

Max parameter length fetched from device#3032
9prady9 merged 2 commits into
arrayfire:masterfrom
willyborn:ParameterLengthOverflow

Conversation

willyborn commented Oct 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Oct 26, 2020

Copy link
Copy Markdown
Member

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 changed the title Parameter length overflow during opencl program build Max parameter length fetched from device Oct 27, 2020
willyborn force-pushed the ParameterLengthOverflow branch 4 times, most recently from 682fa65 to 08d780b Compare October 27, 2020 19:12

9prady9 commented Oct 29, 2020

Copy link
Copy Markdown
Member

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 reviewed Oct 29, 2020
View reviewed changes

9prady9 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good.

Comment thread 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;

9prady9 Oct 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This and the earlier line as essentially identical as device returned

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 force-pushed the ParameterLengthOverflow branch from a219ab9 to 4e2b5ff Compare November 1, 2020 21:15
9prady9 requested changes Nov 2, 2020
View reviewed changes

9prady9 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 merged commit 0493478 into arrayfire:master Nov 4, 2020
9prady9 pushed a commit to 9prady9/arrayfire that referenced this pull request Aug 2, 2021
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
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 deleted the ParameterLengthOverflow branch September 29, 2022 13:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

umar456 umar456 approved these changes
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

Back | FazBrowse Home | New Git URL