[XLA:GPU] Add sycl platform #11425

ShengYang1 · 2024-04-11T03:42:18Z

It is a sub PR of #9042 to add sycl platform

thomasjoerg · 2024-05-06T12:50:46Z

golechwierowicz

LGTM, but I might be lacking some context. @penpornk could you take a look, I see you were a reviewer of the bigger PR stream.

penpornk

So sorry for the delay! I have minor comments.

penpornk · 2024-05-07T11:27:05Z

xla/stream_executor/sycl/BUILD

@@ -0,0 +1,86 @@
+# Description:
+#   SYCL-platform specific StreamExecutor support code.
+# buildifier: disable=out-of-order-load


Nit: This line is duplicated. Please remove.

penpornk · 2024-05-07T11:28:33Z

xla/stream_executor/sycl/BUILD

+
+cc_library(
+    name = "sycl_rpath",
+    data = [],


Nit: This line can be removed.

penpornk · 2024-05-07T11:29:36Z

xla/stream_executor/sycl/BUILD

+    linkopts = select({
+        "//conditions:default": [
+            "-Wl,-rpath,../local_config_sycl/sycl/sycl/lib",
+        ],
+    }),


This can be simplified to

Suggested change

linkopts = select({

"//conditions:default": [

"-Wl,-rpath,../local_config_sycl/sycl/sycl/lib",

],

}),

linkopts = ["-Wl,-rpath,../local_config_sycl/sycl/sycl/lib"],

penpornk · 2024-05-07T11:29:49Z

xla/stream_executor/sycl/BUILD

+            "-Wl,-rpath,../local_config_sycl/sycl/sycl/lib",
+        ],
+    }),
+    deps = [],


Nit: This line can be removed.

penpornk · 2024-05-07T11:31:04Z

xla/stream_executor/sycl/BUILD

+    deps = if_sycl_is_configured([
+        ":sycl_platform",
+    ]),


Nit: Let's make this more compact since there is only one dependency right now.

Suggested change

deps = if_sycl_is_configured([

":sycl_platform",

]),

deps = if_sycl_is_configured([":sycl_platform"]),

penpornk · 2024-05-07T11:32:54Z

xla/stream_executor/sycl/sycl_platform.cc

+
+SyclPlatform::~SyclPlatform() {}
+
+// Due to legacy issues in user code, we can't currently call InpectNumaNodes


Nit: Typo

Suggested change

// Due to legacy issues in user code, we can't currently call InpectNumaNodes

// Due to legacy issues in user code, we can't currently call InspectNumaNodes

penpornk · 2024-05-07T11:33:20Z

xla/stream_executor/sycl/sycl_platform.cc

+
+SyclPlatform::~SyclPlatform() {}
+
+// Due to legacy issues in user code, we can't currently call InpectNumaNodes


Could you please explain more about these legacy issues? Are they the same as CUDA and ROCm?

penpornk · 2024-05-07T11:45:04Z

xla/stream_executor/sycl/sycl_platform.cc

+// Due to legacy issues in user code, we can't currently call InpectNumaNodes
+// at module initialization time, because non-GPU programs still include this
+// plugin via various methods, so instead, it has to be init-on-reference.
+void SyclPlatform::InspectNumaNodes() {


InspectNumaNodes, BusCount, DeviceToBus, FirstExecutorForBus, DescriptionForDevice, ExecutorForDevice, GetExecutor, and GetUncachedExecutor seem pretty similar across CUDA/ROCm/SYCL platforms. Just a heads up that I will refactor them to a base class later.

penpornk · 2024-05-07T11:50:33Z

xla/stream_executor/sycl/sycl_platform.h

+
+  // The smallest NUMA node value for any device managed by this machine
+  // manager. Used, along with limit_numa_node_, to convert NUMA nodes into bus
+  // ordinals. The NUMA node space occupied by GPUs is assumed to be dense./


Nit: Extra character.

Suggested change

// ordinals. The NUMA node space occupied by GPUs is assumed to be dense./

// ordinals. The NUMA node space occupied by GPUs is assumed to be dense.

penpornk · 2024-05-07T11:51:50Z

xla/stream_executor/sycl/sycl_platform.h

+using SyclPlatform = gpu::SyclPlatform;
+
+}  // namespace sycl
+


Nit: Let's remove this extra blank line.

ShengYang1 · 2024-05-08T01:56:22Z

So sorry for the delay! I have minor comments.

Thanks:) I have addressed all of your comments.

penpornk

Thank you! One more nit please. :)

P.S. In the future, could you please avoid force-pushing (i.e., replacing the original commit with a new one)? Committing new changes as new commits on top of the original one helps Github highlight changes since my last review for me.

penpornk · 2024-05-08T04:17:14Z

xla/stream_executor/sycl/BUILD

@@ -0,0 +1,70 @@
+load(


When I said that the line is duplicated, I only meant the # buildifier: disable=out-of-order-load line. (There were two of them.) Please add this back:

# Description: # SYCL-platform specific StreamExecutor support code.

(We can remove the buildifier tag if it isn't needed.)

Got it. Done.

penpornk

Thank you!

Imported from GitHub PR openxla/xla#11425 It is a sub PR of openxla/xla#9042 to add sycl platform Copybara import of the project: -- 7d0c37057c673f784089ce961d24276a496d43bd by Sheng, Yang <yang.sheng@intel.com>: Add sycl platform -- 8d1196831a20f69ba8880d8549007358928f6be1 by Sheng, Yang <yang.sheng@intel.com>: Add comments back Merging this change closes #11425 Reverts f5ff233 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#11425 from Intel-tensorflow:yang/sycl-platform 8d1196831a20f69ba8880d8549007358928f6be1 PiperOrigin-RevId: 631712809

Imported from GitHub PR openxla/xla#11425 It is a sub PR of openxla/xla#9042 to add sycl platform Copybara import of the project: -- 7d0c37057c673f784089ce961d24276a496d43bd by Sheng, Yang <yang.sheng@intel.com>: Add sycl platform -- 8d1196831a20f69ba8880d8549007358928f6be1 by Sheng, Yang <yang.sheng@intel.com>: Add comments back Merging this change closes #11425 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#11425 from Intel-tensorflow:yang/sycl-platform 8d1196831a20f69ba8880d8549007358928f6be1 PiperOrigin-RevId: 631712809

NaiyerRizz · 2024-05-14T09:58:07Z

Hi @penpornk
This PR has been approved but not yet merged, Can you please look into this.

Imported from GitHub PR openxla/xla#11425 It is a sub PR of openxla/xla#9042 to add sycl platform Copybara import of the project: -- 7d0c37057c673f784089ce961d24276a496d43bd by Sheng, Yang <yang.sheng@intel.com>: Add sycl platform -- 8d1196831a20f69ba8880d8549007358928f6be1 by Sheng, Yang <yang.sheng@intel.com>: Add comments back Merging this change closes #11425 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#11425 from Intel-tensorflow:yang/sycl-platform 8d1196831a20f69ba8880d8549007358928f6be1 PiperOrigin-RevId: 634288679

dimitar-asenov · 2024-05-16T13:39:15Z

I'm looking into this, there's some google internal bits that we need to add on our end.

Imported from GitHub PR openxla/xla#11425 It is a sub PR of openxla/xla#9042 to add sycl platform Copybara import of the project: -- 7d0c37057c673f784089ce961d24276a496d43bd by Sheng, Yang <yang.sheng@intel.com>: Add sycl platform -- 8d1196831a20f69ba8880d8549007358928f6be1 by Sheng, Yang <yang.sheng@intel.com>: Add comments back Merging this change closes #11425 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#11425 from Intel-tensorflow:yang/sycl-platform 8d1196831a20f69ba8880d8549007358928f6be1 PiperOrigin-RevId: 634288679

dimitar-asenov · 2024-05-21T08:21:32Z

@ddunl, could you please take a look at the internal errors? I think we need to adjust the copybara config, but I'm not sure what's the right tweak.

ShengYang1 changed the title ~~Add sycl platform~~ [XLA:GPU] Add sycl platform Apr 11, 2024

github-actions bot added the kokoro:force-run Forces CI to rerun label Apr 11, 2024

github-actions bot assigned kamaljeeti and xla-rotation Apr 11, 2024

kokoro-team removed the kokoro:force-run Forces CI to rerun label Apr 11, 2024

ShengYang1 force-pushed the yang/sycl-platform branch from c809f0d to 9aae8eb Compare April 11, 2024 03:46

github-actions bot added the kokoro:force-run Forces CI to rerun label Apr 11, 2024

kokoro-team removed the kokoro:force-run Forces CI to rerun label Apr 11, 2024

Zantares mentioned this pull request Apr 11, 2024

[Draft][XLA:GPU] Add sycl runtime #9042

Draft

kamaljeeti requested a review from golechwierowicz April 12, 2024 05:33

golechwierowicz requested a review from penpornk April 12, 2024 09:34

golechwierowicz reviewed May 6, 2024

View reviewed changes

penpornk suggested changes May 7, 2024

View reviewed changes

Add sycl platform

7d0c370

ShengYang1 force-pushed the yang/sycl-platform branch from 9aae8eb to 7d0c370 Compare May 8, 2024 01:52

github-actions bot added the kokoro:force-run Forces CI to rerun label May 8, 2024

kokoro-team removed the kokoro:force-run Forces CI to rerun label May 8, 2024

penpornk suggested changes May 8, 2024

View reviewed changes

Add comments back

8d11968

github-actions bot added the kokoro:force-run Forces CI to rerun label May 8, 2024

kokoro-team removed the kokoro:force-run Forces CI to rerun label May 8, 2024

penpornk approved these changes May 8, 2024

View reviewed changes

copybara-service bot mentioned this pull request May 8, 2024

PR #11425: [XLA:GPU] Add sycl platform tensorflow/tensorflow#67156

Closed

copybara-service bot mentioned this pull request May 16, 2024

PR #11425: [XLA:GPU] Add sycl platform tensorflow/tensorflow#67728

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[XLA:GPU] Add sycl platform #11425

[XLA:GPU] Add sycl platform #11425

ShengYang1 commented Apr 11, 2024

thomasjoerg commented May 6, 2024

golechwierowicz left a comment

penpornk left a comment

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

penpornk May 7, 2024

ShengYang1 commented May 8, 2024

penpornk left a comment

penpornk May 8, 2024

ShengYang1 May 8, 2024

penpornk left a comment

NaiyerRizz commented May 14, 2024

dimitar-asenov commented May 16, 2024

dimitar-asenov commented May 21, 2024


		SyclPlatform::~SyclPlatform() {}

		// Due to legacy issues in user code, we can't currently call InpectNumaNodes

	// ordinals. The NUMA node space occupied by GPUs is assumed to be dense./
	// ordinals. The NUMA node space occupied by GPUs is assumed to be dense.

[XLA:GPU] Add sycl platform #11425

Are you sure you want to change the base?

[XLA:GPU] Add sycl platform #11425

Conversation

ShengYang1 commented Apr 11, 2024

thomasjoerg commented May 6, 2024

golechwierowicz left a comment

Choose a reason for hiding this comment

penpornk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShengYang1 commented May 8, 2024

penpornk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penpornk left a comment

Choose a reason for hiding this comment

NaiyerRizz commented May 14, 2024

dimitar-asenov commented May 16, 2024

dimitar-asenov commented May 21, 2024