[SYCL] Change priority of devices in default_selector #1264

jbrodman · 2020-03-06T15:04:35Z

People are unlikely to want to accidentally stumble onto the fpga.

Signed-off-by: James Brodman james.brodman@intel.com

mkinsner

Also want approval by @bader

bader · 2020-03-06T16:08:01Z

This change breaks LIT tests. We need some way to use FPGA "intentionally".

jbrodman · 2020-03-06T16:11:09Z

queue(accelerator_selector{});

bader · 2020-03-06T16:13:14Z

queue(accelerator_selector{});

SGTM, if you are willing to update the tests, so we can avoid LIT placeholders.

jbrodman · 2020-03-06T16:17:17Z

queue(accelerator_selector{});

SGTM, if you are willing to update the tests, so we can avoid LIT placeholders.

Working on it.

bader · 2020-03-06T16:19:28Z

So, you think we should not be able to configure default selector though environment variable. Right?

jbrodman · 2020-03-06T16:24:32Z

So, you think we should not be able to configure default selector though environment variable. Right?

Which would you and Mike prefer? No Accel in default (and therefore no ENV override for ACC) or Accel after GPU/CPU but before host? Hmm... if accel is after host, would the override still pick ACC? That might be the best path.

@bader, @mkinsner

jbrodman · 2020-03-06T16:29:54Z

So, you think we should not be able to configure default selector though environment variable. Right?

Which would you and Mike prefer? No Accel in default (and therefore no ENV override for ACC) or Accel after GPU/CPU but before host? Hmm... if accel is after host, would the override still pick ACC? That might be the best path.

@bader, @mkinsner

That seems to work quite well, actually.

mkinsner · 2020-03-06T16:36:36Z

Accel after (lower priority than) host device makes sense to me.

keryell · 2020-03-07T02:14:40Z

Hey, you removed the secret backdoor used to sell more FPGA in the datacenters! ;-)

KarachunIvan

Why does an accelerator have lower priority than host device? According to your changes, any accelerator device won't be available at all while using default_selector since host is always presented.
IMHO, host device should be the least desired option and this should be reflected in default_selector.
Wouldn't it be better to swap priority of cpu and accelerator instead?

jbrodman · 2020-03-09T14:50:47Z

Why does an accelerator have lower priority than host device? According to your changes, any accelerator device won't be available at all while using default_selector since host is always presented.
IMHO, host device should be the least desired option and this should be reflected in default_selector.
Wouldn't it be better to swap priority of cpu and accelerator instead?

B/c it's unlikely that people want to accidentally stumble on an "accel" device. Putting it after host should still allow it to appear when SYCL_DEVICE_TYPE=ACC is set, right?

KarachunIvan · 2020-03-09T17:57:25Z

Why does an accelerator have lower priority than host device? According to your changes, any accelerator device won't be available at all while using default_selector since host is always presented.
IMHO, host device should be the least desired option and this should be reflected in default_selector.
Wouldn't it be better to swap priority of cpu and accelerator instead?

B/c it's unlikely that people want to accidentally stumble on an "accel" device. Putting it after host should still allow it to appear when SYCL_DEVICE_TYPE=ACC is set, right?

Yes, it'll appear but it'll never be chosen by select_device method which picks a device with the highest score according to the score returned by default_selector::operator()() with environment variable set or without.

What about something like this:

if (is_accelerator() && std::getenv("SYCL_DEVICE_TYPE"))
  return 400;

I've just come up with this, and it can solve several issues:

a user won't stumble on an accelerator device (if he won't set the environment variable);
no need to change the whole bunch of failing LIT tests (because in fact they were executed on host but not on an accelerator).

jbrodman · 2020-03-09T18:03:18Z

@KarachunIvan how 'bout that? Override always wins.

jbrodman · 2020-03-09T18:07:56Z

@KarachunIvan how 'bout that? Override always wins.

With that, it probably makes sense to just remove the if(dev.is_accelerator()) part.

GarveyJoe · 2020-03-09T18:20:06Z

sycl/source/device_selector.cpp

@@ -54,18 +55,22 @@ int default_selector::operator()(const device &dev) const {
    }
  }

+  // override always wins
+  if (dev.get_info<info::device::device_type>() == detail::get_forced_type())


I think when the device type is forced we only return devices of the forced type. Thus, this is redundant. Can someone who knows the device internals better confirm that?

We don't actually do that apparently. We will limit the devices enumerated to that type plus the host device since the host device must always be available. In that case, the fact that the host device is available with higher priority means it gets selected.

bso-intel · 2020-03-10T00:33:17Z

@KarachunIvan how 'bout that? Override always wins.

With that, it probably makes sense to just remove the if(dev.is_accelerator()) part.

I agree with removing that part.
If host is ALWAYS available, the if (dev.is_accelerator()) is an unreachable code. Makes sense to remove it.
This practically means that ACC(FPGA) is never be chosen by default, and the only way is to set the env var SYCL_DEVICE_TYPE=ACC.

KarachunIvan · 2020-03-10T06:59:10Z

sycl/source/device_selector.cpp

+  if (dev.is_accelerator())
+    return 50;
+


Please remove this unreachable code

romanovvlad · 2020-03-17T15:42:14Z

@jbrodman @KarachunIvan ping

KarachunIvan · 2020-04-03T14:03:13Z

@jbrodman ping

…o accidentally stumble upon a fpga. Signed-off-by: James Brodman <james.brodman@intel.com>

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman · 2020-04-03T14:46:41Z

Sorry, fixed.

KarachunIvan

LGTM

romanovvlad

LGTM

…_private_api * origin/sycl: (614 commits) [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466) [SYCL][USM] Remove vestigial dead code (intel#1474) [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451) [Driver][SYCL] Emit an error if c compilation is forced (intel#1438) [SYCL] Fix sycl-post-link when no split and symbols are requested. (intel#1454) [SYCL] Change priority of devices in default_selector (intel#1264) [CI] Update CODEOWNERS matching rules order (intel#1468) [SYCL] Share PFWG lambda object through shared memory (intel#1455) [CI] Fix CODEOWNERS file syntax (intel#1464) [SYCL][CUDA] Fix active context when creating base event (intel#1447) [SYCL] Diagnose implicit declaration of kernel function type (intel#1450) [BuildBot] Modify configure script (intel#1421) [SYCL] Resolve min/max conflict (intel#1339) [CI][BuildBot] Fix configure parameter to turn on/off assertions (intel#1449) [SYCL] XFAIL LIT test due to duplicate diagnostic [SYCL] Remove explicit sycl_device attribute requirement Apply more suggestions Apply suggestions Translate new set of Intel FPGA Loop Controls Translate Intel FPGA force_pow2_depth memory attribute ...

…c_abi_checks * origin/sycl: (625 commits) [SYCL][Test] Disable spec_const_redefine.cpp on all devices but HOST (intel#1488) [SYCL] Only export public API (intel#1456) [SYCL][CUDA] Fix selected_binary argument in piextDeviceSelectBinary (intel#1475) [SYCL] Enable LIT testing with CUDA BE (intel#1458) [SYCL] Fix float to half-type conversion (intel#1395) [NFC] Cleanup unneded macro from builtins implementation (intel#1445) Enable cfg-printer LLVM lit tests only if LLVM linked statically (intel#1479) [SYCL][NFC] Reflect the "allowlist" renaming in the code (intel#1480) [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466) [SYCL][USM] Remove vestigial dead code (intel#1474) [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451) [Driver][SYCL] Emit an error if c compilation is forced (intel#1438) [SYCL] Fix sycl-post-link when no split and symbols are requested. (intel#1454) [SYCL] Change priority of devices in default_selector (intel#1264) [CI] Update CODEOWNERS matching rules order (intel#1468) [SYCL] Share PFWG lambda object through shared memory (intel#1455) [CI] Fix CODEOWNERS file syntax (intel#1464) [SYCL][CUDA] Fix active context when creating base event (intel#1447) [SYCL] Diagnose implicit declaration of kernel function type (intel#1450) [BuildBot] Modify configure script (intel#1421) ...

jbrodman requested review from bader, romanovvlad and mkinsner March 6, 2020 15:04

mkinsner previously approved these changes Mar 6, 2020

View reviewed changes

jbrodman dismissed mkinsner’s stale review via 968039e March 6, 2020 16:29

jbrodman changed the title ~~[SYCL] Remove accels from default selector as people aren't likely to want t…~~ [SYCL] Change priority of devices in default_selector Mar 6, 2020

AGindinson requested a review from GarveyJoe March 6, 2020 16:33

bader previously approved these changes Mar 6, 2020

View reviewed changes

mkinsner previously approved these changes Mar 6, 2020

View reviewed changes

GarveyJoe previously approved these changes Mar 6, 2020

View reviewed changes

bader assigned KarachunIvan Mar 8, 2020

KarachunIvan suggested changes Mar 9, 2020

View reviewed changes

jbrodman dismissed stale reviews from GarveyJoe, mkinsner, and bader via 61bfe30 March 9, 2020 17:40

jbrodman force-pushed the defaultselector branch from 968039e to 61bfe30 Compare March 9, 2020 17:40

GarveyJoe reviewed Mar 9, 2020

View reviewed changes

KarachunIvan suggested changes Mar 10, 2020

View reviewed changes

jbrodman added 4 commits April 3, 2020 10:31

Remove accels from default selector as people aren't likely to want t…

20fa2a9

…o accidentally stumble upon a fpga. Signed-off-by: James Brodman <james.brodman@intel.com>

Change to preferring host to accel

040edc2

Signed-off-by: James Brodman <james.brodman@intel.com>

Make the override always win

4df0958

Signed-off-by: James Brodman <james.brodman@intel.com>

Remove redundant code and rebase

e8f6a0f

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman force-pushed the defaultselector branch from d0fa3bb to e8f6a0f Compare April 3, 2020 14:46

KarachunIvan approved these changes Apr 3, 2020

View reviewed changes

romanovvlad approved these changes Apr 3, 2020

View reviewed changes

bader merged commit b217bc5 into intel:sycl Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Change priority of devices in default_selector #1264

[SYCL] Change priority of devices in default_selector #1264

jbrodman commented Mar 6, 2020 •

edited

Loading

mkinsner left a comment

bader commented Mar 6, 2020

jbrodman commented Mar 6, 2020 •

edited

Loading

bader commented Mar 6, 2020

jbrodman commented Mar 6, 2020

bader commented Mar 6, 2020

jbrodman commented Mar 6, 2020 •

edited

Loading

jbrodman commented Mar 6, 2020

mkinsner commented Mar 6, 2020

keryell commented Mar 7, 2020

KarachunIvan left a comment

jbrodman commented Mar 9, 2020

KarachunIvan commented Mar 9, 2020

jbrodman commented Mar 9, 2020

jbrodman commented Mar 9, 2020

GarveyJoe Mar 9, 2020

jbrodman Mar 9, 2020

bso-intel commented Mar 10, 2020 •

edited

Loading

KarachunIvan Mar 10, 2020

romanovvlad commented Mar 17, 2020

KarachunIvan commented Apr 3, 2020

jbrodman commented Apr 3, 2020

KarachunIvan left a comment

romanovvlad left a comment

[SYCL] Change priority of devices in default_selector #1264

[SYCL] Change priority of devices in default_selector #1264

Conversation

jbrodman commented Mar 6, 2020 • edited Loading

mkinsner left a comment

Choose a reason for hiding this comment

bader commented Mar 6, 2020

jbrodman commented Mar 6, 2020 • edited Loading

bader commented Mar 6, 2020

jbrodman commented Mar 6, 2020

bader commented Mar 6, 2020

jbrodman commented Mar 6, 2020 • edited Loading

jbrodman commented Mar 6, 2020

mkinsner commented Mar 6, 2020

keryell commented Mar 7, 2020

KarachunIvan left a comment

Choose a reason for hiding this comment

jbrodman commented Mar 9, 2020

KarachunIvan commented Mar 9, 2020

jbrodman commented Mar 9, 2020

jbrodman commented Mar 9, 2020

GarveyJoe Mar 9, 2020

Choose a reason for hiding this comment

jbrodman Mar 9, 2020

Choose a reason for hiding this comment

bso-intel commented Mar 10, 2020 • edited Loading

KarachunIvan Mar 10, 2020

Choose a reason for hiding this comment

romanovvlad commented Mar 17, 2020

KarachunIvan commented Apr 3, 2020

jbrodman commented Apr 3, 2020

KarachunIvan left a comment

Choose a reason for hiding this comment

romanovvlad left a comment

Choose a reason for hiding this comment

jbrodman commented Mar 6, 2020 •

edited

Loading

jbrodman commented Mar 6, 2020 •

edited

Loading

jbrodman commented Mar 6, 2020 •

edited

Loading

bso-intel commented Mar 10, 2020 •

edited

Loading