Skip to content
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

Remove command line parameters for platform and device selection #1073

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ellnor01
Copy link
Contributor

Fixes #711

Signed-off-by: Ellen Norris-Thompson ellen.norris-thompson@arm.com

@gwawiork
Copy link
Contributor

Hi.
This change will impact our automated execution. Is there really need to remove this kind of selection without introducing new solution?

@kpet
Copy link
Contributor

kpet commented Dec 9, 2020

There is already another way of selecting devices and platforms using the CL_DEVICE_INDEX, CL_PLATFORM_INDEX and CL_DEVICE_TYPE environment variables. The command-line syntax is quirky and will get in the way of introducing a cleaner command-line parsing logic.

@ellnor01 The help text there would need updating as well.

@kpet
Copy link
Contributor

kpet commented Jan 14, 2021

@gwawiork Any more feedback on this change?

@nikhiljnv
Copy link
Contributor

There is already another way of selecting devices and platforms using the CL_DEVICE_INDEX, CL_PLATFORM_INDEX and CL_DEVICE_TYPE environment variables. The command-line syntax is quirky and will get in the way of introducing a cleaner command-line parsing logic.

@ellnor01 The help text there would need updating as well.

I would prefer command-line arguments over env var as relying on env vars can be fragile (it's easy to miss setting them up etc).
Also, going bit further, just platform_index and device_index doesn't guarantee portability of these indices across different systems due to dependence on enumeration order. We had to add a check for platform name to ensure we indeed run on the intended platform irrespective or enumeration order.

@kpet
Copy link
Contributor

kpet commented Feb 5, 2021

@nikhiljnv Agree that's where we want to end up but currently we have a mix of environment variables and command line parameters for device/platform configuration (with both supported in some cases!) and the command line syntax is non-standard/cryptic/atrocious so we will have to break the existing command line interface. What I'm proposing we do is:

  1. Remove the current command line syntax and use environment variables for all things device/platform configuration (already the case for device types). That's this PR. This lowers a bar to re-design command line parsing.
  2. Re-introduce a clean command line interface when we get to re-designing command line parsing.
  3. Deprecate and remove all the environment variables.

It does mean two breaking changes but we can stage them.

Fixes KhronosGroup#711

Signed-off-by: Ellen Norris-Thompson <ellen.norris-thompson@arm.com>
Removing text describing how to use command line parameters
which have been removed

Contributes KhronosGroup#711

Signed-off-by: Ellen Norris-Thompson <ellen.norris-thompson@arm.com>
@ahesham-arm
Copy link
Contributor

@kpet Rebased and CI checks are passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove command line parameters for platform and device selection
5 participants