[SYCL] Device config file implementation #9846

maarquitos14 · 2023-06-13T15:15:35Z

Prototype implementation for default Device Config File designed in #9371. Part of the testing is not present yet, nor is the ability to extend the default config file.

AlexeySachkov

First iteration for now, but I will do a second pass, more comments are to be expected

clang/lib/Driver/ToolChains/Clang.cpp

sycl/include/sycl/device_config_file.hpp

sycl/include/sycl/device_config_file.td

llvm/utils/TableGen/DynamicTableEmitter.cpp

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

clang/lib/Driver/ToolChains/Clang.cpp

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td

AlexeySachkov · 2023-06-20T09:30:27Z

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td

+  string aotToolchainOptions = options;
+}
+
+// This definition serves the only purpose of testing wether the aspect list defined in here and in SYCL RT matches


If we are going to make clang driver to use list of targets defined here as a filter for supported targets, then we should probably somehow make sure that those are not exposed to user.

They at least should not be visible in any help and ideally should not be allowed to be set by a user at all. However, as a bare minimum, we could name them starting with double underscore or something like that to clearly indicate that this is not something to use by an end user.

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td

AlexeySachkov · 2023-06-20T09:37:30Z

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td

+// This definition serves the only purpose of testing wether the deprecated aspect list defined in here and in SYCL RT matches
+def : TargetInfo<"TestDeprecatedAspectList", 0, [AspectInt64_base_atomics, AspectInt64_extended_atomics, AspectUsm_system_allocator, AspectUsm_restricted_shared_allocations, AspectHost], [], "", "">;
+
+def : TargetInfo<"TargetA", 1, [AspectCpu, AspectAtomic64], [8, 16], "ocloc", "devA">;


I wonder, if we need to have an ability to pass "shortcuts" to list of aspects. What I mean here is that for most of devices there will be a set of "core" aspects, which is the same. To avoid repeating it for every target, we could say something like:

def AllUSMAspects: [AspectUsm_device_allocations, AspectUsm_host_allocations, AspectUsm_shared_allocations, AspectUsm_system_allocations, AspectUsm_atomic_host_allocations AspectUsm_atomic_shared_allocations]; def : TargetInfo<"TargetA", 1, [AspectCpu, AspectAtomic64. AllUSMAspects], [8, 16], "ocloc", "devA">;

However, I'm not sure how huge that "core" set of aspects would be or how huge would be groups which we can create, i.e. I'm not entirely sure if the repetition of the same aspect would be a real problem.

With the current status of things, we can do:

defvar AllUSMAspects = [AspectUsm_device_allocations, AspectUsm_host_allocations, AspectUsm_shared_allocations, AspectUsm_system_allocations, AspectUsm_atomic_host_allocations, AspectUsm_atomic_shared_allocations]; def : TargetInfo<"Test", AllUSMAspects, []>; defvar AspectList = [AspectCpu] # AllUSMAspects; def : TargetInfo<"Test2", AspectList, []>; def : TargetInfo<"Test3", [AspectCpu] # AllUSMAspects, []>;

llvm/utils/TableGen/DynamicTableEmitter.cpp

AlexeySachkov · 2023-06-20T10:27:34Z

llvm/utils/TableGen/DynamicTableEmitter.cpp

+  // The primary data table contains all the fields defined for this map.
+  OS << "std::map<std::string, " << Table.CppTypeName << "> " << Table.Name
+     << " = {\n";
+  for (unsigned i = 0; i < Table.Entries.size(); ++i) {


Suggested change

for (unsigned i = 0; i < Table.Entries.size(); ++i) {

for (unsigned I = 0; I < Table.Entries.size(); ++I) {

AlexeySachkov · 2023-06-20T10:31:56Z

llvm/utils/TableGen/DynamicTableEmitter.cpp

+      if (first) {
+        first = false;
+        second = true;
+        OS << LS << val << ", { ";
+      } else {
+        if (!second) {
+          OS << LS;
+        }
+        if (Field.IsList) {
+          val.insert(0, 1, '{');
+          val.push_back('}');
+        }
+        OS << val;
+        second = false;


This logic is a bit hard to understand at first glance. I think that there are some possibilities to do refactoring and simplify it, but it is hard to suggest without understanding the output here.

Which brings me to an important point: there are no tests for this new tablegen backend and we should add some. See llvm/test/TableGen for examples of existing ones

llvm/utils/TableGen/DynamicTableEmitter.cpp

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

steffenlarsen

Tiny nit, but otherwise I think it looks ready to go out of draft!

steffenlarsen · 2023-06-21T13:35:23Z

llvm/utils/TableGen/DynamicTableEmitter.cpp

+      if (!IsKey)
+        OS << TargetInfoElemSeparator << val;
+      else {


Nit: I prefer having either all branches having bodies or none of them having.

Suggested change

if (!IsKey)

OS << TargetInfoElemSeparator << val;

else {

if (!IsKey) {

OS << TargetInfoElemSeparator << val;

} else {

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.hpp

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

steffenlarsen

Runtime changes LGTM!

maarquitos14 · 2023-06-27T13:07:59Z

Friendly ping @intel/dpcpp-esimd-reviewers.

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.hpp

AlexeySachkov · 2023-06-27T13:47:26Z

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td

+
+def : TargetInfo<"TargetA", [AspectCpu, AspectAtomic64], [8, 16], "ocloc", "devA", 1>;
+def : TargetInfo<"TargetB", [AspectGpu, AspectFp16], [8, 16], "ocloc", "devB">;
+def : TargetInfo<"TargetC", [AspectEmulated, AspectImage], [8, 16], "ocloc", "devC">;
+def : TargetInfo<"TargetD", [AspectCpu, AspectFp64, AspectHost_debuggable], [8, 16], "ocloc", "devD", 1>;
+defvar AspectList = [AspectCpu] # AllUSMAspects;
+def : TargetInfo<"Test", AspectList, []>;
+def : TargetInfo<"Test2", [AspectCpu] # AllUSMAspects, []>;


I understand that those are examples, but we should either remove them or move to comments before the commit.

I think it makes sense to leave one simple example of adding a new target here and one example, where list of aspects is produced as a combination of explicitly specified values + some other predefined list.

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td

llvm/utils/TableGen/DynamicTableEmitter.cpp

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

sarnex

llvm/lib/SYCLLowerIR/CMakeLists.txt LGTM from ESIMD side

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

…config-file

maksimsab

LGTM

Prototype implementation for Device Config File.

d4fae2d

maarquitos14 requested review from AlexeySachkov and steffenlarsen June 13, 2023 15:16

AlexeySachkov reviewed Jun 13, 2023

View reviewed changes

Addressing feedback from code review.

65d991e

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 commented Jun 14, 2023

View reviewed changes

clang/lib/Driver/ToolChains/Clang.cpp Outdated Show resolved Hide resolved

Tweak build system to run Tablegen for Device Config File.

1315da8

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 16, 2023 15:27 — with GitHub Actions Inactive

maarquitos14 added 2 commits June 16, 2023 08:56

Remove unnecessary includes.

58c1a6d

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Update backend description.

a80767e

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 16, 2023 16:23 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 16, 2023 17:04 — with GitHub Actions Inactive

maarquitos14 added 2 commits June 19, 2023 07:51

Emit aspect name rather than identifier.

e766cb0

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Add test to check if Device Config File and SYCL RT aspect lists match.

2a783f6

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 19, 2023 15:37 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 19, 2023 16:18 — with GitHub Actions Inactive

AlexeySachkov reviewed Jun 20, 2023

View reviewed changes

Addressing feedback from code review.

f355b21

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 20, 2023 16:36 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 20, 2023 23:12 — with GitHub Actions Inactive

maarquitos14 added 2 commits June 21, 2023 06:39

Add tests Dynamic Table tablegen backend.

8fc6aa6

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Fix clang-format issues.

241d064

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

steffenlarsen reviewed Jun 21, 2023

View reviewed changes

maarquitos14 added 4 commits June 21, 2023 06:46

Address code review feedback.

706f792

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Removing demo code from diver.

f838f37

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Remove unnecessary include.

468e2d5

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Remove unnecessary includes.

c5137e7

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 marked this pull request as ready for review June 21, 2023 14:11

maarquitos14 requested review from a team as code owners June 21, 2023 14:11

maarquitos14 temporarily deployed to aws June 21, 2023 16:56 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 21, 2023 18:10 — with GitHub Actions Inactive

Adding include required in MSVC.

c2ec1b9

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 22, 2023 10:17 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 22, 2023 10:54 — with GitHub Actions Inactive

maksimsab requested changes Jun 23, 2023

View reviewed changes

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.hpp Show resolved Hide resolved

Address code review feedback.

860f3bf

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 26, 2023 17:15 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 26, 2023 18:16 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 26, 2023 19:34 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 26, 2023 22:13 — with GitHub Actions Inactive

steffenlarsen approved these changes Jun 27, 2023

View reviewed changes

AlexeySachkov reviewed Jun 27, 2023

View reviewed changes

Addressing code review feedback.

27a3387

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 28, 2023 14:49 — with GitHub Actions Inactive

AlexeySachkov approved these changes Jun 28, 2023

View reviewed changes

AlexeySachkov requested a review from maksimsab June 28, 2023 15:33

sarnex approved these changes Jun 28, 2023

View reviewed changes

Fix test issue.

d88eaf8

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

maarquitos14 temporarily deployed to aws June 28, 2023 17:12 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 28, 2023 19:34 — with GitHub Actions Inactive

Merge remote-tracking branch 'intel/origin/sycl' into maronas/device-…

309d784

…config-file

maarquitos14 temporarily deployed to aws June 28, 2023 20:50 — with GitHub Actions Inactive

maarquitos14 temporarily deployed to aws June 28, 2023 22:22 — with GitHub Actions Inactive

maarquitos14 changed the title ~~[SYCL] Device config file prototype~~ [SYCL] Device config file implementation Jun 29, 2023

maksimsab approved these changes Jun 29, 2023

View reviewed changes

steffenlarsen merged commit 0bb7221 into intel:sycl Jun 29, 2023

Chenyang-L mentioned this pull request Jul 11, 2023

LLVM and SPIRV-LLVM-Translator pulldown (WW25) #10311

Closed

AlexeySachkov mentioned this pull request Jul 24, 2023

opencl-aot fails to compile SYCL kernels with an unsupported subgroup size #10531

Open

	for (unsigned i = 0; i < Table.Entries.size(); ++i) {
	for (unsigned I = 0; I < Table.Entries.size(); ++I) {

[SYCL] Device config file implementation #9846

[SYCL] Device config file implementation #9846

Uh oh!

Conversation

maarquitos14 commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexeySachkov Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

maarquitos14 Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

maarquitos14 commented Jun 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexeySachkov Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

maksimsab left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maarquitos14 commented Jun 13, 2023 •

edited

Loading