[SYCL] SemaSYCL significant refactoring #1517

Fznamznon · 2020-04-14T13:55:18Z

This is part of refactoring started here - #1495 .

The integration header and OpenCL generation code has evolved over time to become convoluted and difficult to maintain. It doesn't properly handle the recursive nature of types in many cases as it seems to do so on a mostly ad-hoc basis. It also has a fairly sizable use of recursive lambdas that are both difficult to read and painful to modify. The variety of checks and processings are unfortunately mixed to the point where it is difficult to follow the path through the code.

This patch replaces the existing implementation with a series of visitor types and infrastructure with which to properly iterate and recurse through the kernel lambda objects. It also better splits up the responsibilities through the visitor types, with a separate one for type checking, kernel declaration generation, kernel body generation, and integration header generation. These types have minimal interaction between them.

The visitor pattern also vastly simplifies how each type is handled, since each type has its own handling function, it is only required for an implementer of the visitor to handle one type at a time.

Overall, we hope that this will result in a significantly easier to maintain, modify, and review infrastructure for this functionality.

Signed-off-by: Erich Keane <erich.keane@intel.com>

…or. Still need IntHeader and kernel body generation Signed-off-by: Erich Keane <erich.keane@intel.com>

Signed-off-by: Erich Keane <erich.keane@intel.com>

…e :)

Signed-off-by: Erich Keane <erich.keane@intel.com>

…ill need to fix the integration header offset calculation/keeping track Signed-off-by: Erich Keane <erich.keane@intel.com>

…. Quitting for the weekend, pushing so that others can have a look at the progress/help how they can

Signed-off-by: Erich Keane <erich.keane@intel.com>

…y creator to get the list of parameters for the currently handled field Signed-off-by: Erich Keane <erich.keane@intel.com>

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

erichkeane

This seems like a good first run. It would be nice if we could minimize the amount of data each of these had to store.

Otherwise, good job!

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

This reverts commit c4da2ce.

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Fznamznon · 2020-04-16T17:05:51Z

Hm, something is wrong with unnamed lambdas.

erichkeane · 2020-04-16T17:11:40Z

Hm, something is wrong with unnamed lambdas.

I haven't been building this lately, is there a test fail you're referring to? Do you want me to take a look?

erichkeane · 2020-04-20T18:24:58Z

@elizabethandrews : wanna give this a once over for me too? Otherwise @bader, I think this is likely ready!

Note the check_pr test failure is because I didn't sign some of the patches, but a merge will squash them anyway.

elizabethandrews · 2020-04-20T18:27:35Z

@elizabethandrews : wanna give this a once over for me too? Otherwise @bader, I think this is likely ready!

Note the check_pr test failure is because I didn't sign some of the patches, but a merge will squash them anyway.

Yes. I'm taking a look. I'm not as fast as you both! So you end up saying what I had flagged for review before I post :)

bader

While we are still reviewing - a couple of minor comments from me.
I think we need further simplify this file, but it's a separate PR.

bader · 2020-04-20T18:31:33Z

clang/test/CodeGenSYCL/integration_header.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang -I %S/Inputs -fsycl-device-only -Xclang -fsycl-int-header=%t.h %s -c -o %T/kernel.spv
+// RUN: %clang -I %S/Inputs -fsycl-device-only -Xclang -fsycl-int-header=%t.h %s -fsyntax-only -o %T/kernel.spv


Suggested change

// RUN: %clang -I %S/Inputs -fsycl-device-only -Xclang -fsycl-int-header=%t.h %s -fsyntax-only -o %T/kernel.spv

// RUN: %clang -I %S/Inputs -fsycl-device-only -Xclang -fsycl-int-header=%t.h %s -fsyntax-only

I would also add following TODOs for the follow-up improvements:

I would also bypass the driver and use %clang_cc1.

It's looks strange the we use -fsyntax-only in CodeGen test. I think we should address this by moving integration header generation to CodeGen library.

@Fznamznon for the TODOs.

Do we have a good way of moving it architected. I cannot think of a good way to move integration headers generation later in the process.

As far as using fsyntax-only in the CodeGen test, yes, this is weird. I'm not convinced that this test should BE a CodeGen test as opposed to a SEMA test.

I can't say if there is a good way to move integration header generation to the CodeGen or not, since I don't know all parts of the clang good enough. But I wouldn't refuse considering it.
IMO generation of some code in Sema seems weird for me (it has been weird ever since it appeared, but I didn't come up other suggestion so here it is). See https://clang.llvm.org/docs/InternalsManual.html#the-sema-library, it says that Sema does semantic analysis and builds AST.
I think we can create an issue to investigate this approach (maybe asking the community about it) at least, instead of leaving TODO in the test that no one ever looks at.

You're right that conceptually it could be in CodeGen (though, most call Clang Codegen "IR-Gen", since it doesn't generate 'code', it generates IR).

But I just don't see a good way to do it. And splitting it from the rest of the kernel generation(which it is so closely related to) is perhaps a bad idea as well. We should ask community when we come to open sourcing it, though I'm unconvinced "CodeGen" is the right place.

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews

Just a few nits.

clang/lib/Sema/SemaSYCL.cpp

Since the only caller to this now uses both values, refactor it to just return a pair and unpack on the other side. Signed-off-by: Erich Keane <erich.keane@intel.com>

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

erichkeane

I didn't see the windows test failure, but I'm surprised you had to do that... Did I do something in the test that changed layouts in windows?

Fznamznon · 2020-04-22T13:07:54Z

I didn't see the windows test failure, but I'm surprised you had to do that... Did I do something in the test that changed layouts in windows?

No the windows failure was my fault. I changed test run line so it is run without the clang driver and forgot the triple because with the clang driver it is passed automatically. It caused windows mangling for kernel names and test started failing.

erichkeane · 2020-04-22T13:09:59Z

I didn't see the windows test failure, but I'm surprised you had to do that... Did I do something in the test that changed layouts in windows?

No the windows failure was my fault. I changed test run line so it is run without the clang driver and forgot the triple because with the clang driver it is passed automatically. It caused windows mangling for kernel names and test started failing.

Ah, that makes more sense :) Thanks for clarifying. @bader ready when you are :)

bader · 2020-04-22T13:30:32Z

Could you help me to write a good commit message for this patch, please?
I would appreciate if you could update the description with the summary of the changes done in this refactoring.

erichkeane · 2020-04-22T13:49:25Z

Could you help me to write a good commit message for this patch, please?
I would appreciate if you could update the description with the summary of the changes done in this refactoring.

I edited the description above with my first run at it. Hows that work for you?

bader · 2020-04-22T14:03:54Z

It works for me. Thank you.

…versioning * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536) [SYCL][CUDA] Move interop tests (intel#1570) [Driver][SYCL] Remove COFF object format designator for Windows device compiles (intel#1574) [SYCL] Fix conflicting visibility attributes (intel#1571) [SYCL][DOC] Update the SYCL Runtime Interface document with design details (intel#680) [SYCL] Improve image accessors support on a host device (intel#1502) [SYCL] Make queue's non-USM event ownership temporary (intel#1561) [SYCL] Added support of rounding modes for non-host devices (intel#1463) [SYCL] SemaSYCL significant refactoring (intel#1517) [SYCL] Support 0-dim accessor in handler::copy(accessor, accessor) (intel#1551)

Erich Keane and others added 18 commits April 14, 2020 07:43

WIP on 'visitor' model for the SemaSYCL refactor

3b00883

Signed-off-by: Erich Keane <erich.keane@intel.com>

clang format

2ea64c8

Signed-off-by: Erich Keane <erich.keane@intel.com>

Massive set of additions/changes. Implemneted the declaration generat…

2373337

…or. Still need IntHeader and kernel body generation Signed-off-by: Erich Keane <erich.keane@intel.com>

clang-format changes

4b5abb1

Signed-off-by: Erich Keane <erich.keane@intel.com>

more clang-format changes

3ef0473

Signed-off-by: Erich Keane <erich.keane@intel.com>

A few spelling changes, move the kernel type changing

07bb9be

Signed-off-by: Erich Keane <erich.keane@intel.com>

remove TODO comment, the function has now returned to a managable siz…

ec1e68d

…e :)

WIP with integration header

ec93877

Signed-off-by: Erich Keane <erich.keane@intel.com>

Fix int header 'kind' calculation. Also, make sure we visit bases. St…

e52f39c

…ill need to fix the integration header offset calculation/keeping track Signed-off-by: Erich Keane <erich.keane@intel.com>

small refactoring for base classes

bf828fc

First run at getting the offset correct... not sure if this works

5baa5ad

Got the offset logic compiling/looking right. Also did a clang format…

c551aad

…. Quitting for the weekend, pushing so that others can have a look at the progress/help how they can

Format the createOpenCLKernel func, remove int header from comment

6a5609c

Rename kernelobj to kerneldecl

542a078

Signed-off-by: Erich Keane <erich.keane@intel.com>

Framwork for body, added the stream type as a special type

7d3bb74

Signed-off-by: Erich Keane <erich.keane@intel.com>

Fix all the build errors from the last patch

9f582cd

Signed-off-by: Erich Keane <erich.keane@intel.com>

Add getParamVarDeclsForCurrentField, which provides a way for the bod…

6b987c3

…y creator to get the list of parameters for the currently handled field Signed-off-by: Erich Keane <erich.keane@intel.com>

Implement body generation for accessor, struct and scalar

726212b

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Fznamznon requested a review from erichkeane as a code owner April 14, 2020 13:55

erichkeane reviewed Apr 14, 2020

View reviewed changes

Fznamznon added 8 commits April 14, 2020 17:44

Apply clang format and a couple of comments

3b2e900

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Make LIT tests pass

d642fb9

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Remove const casts

c4da2ce

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Revert "Remove const casts"

d37f1df

This reverts commit c4da2ce.

Add support for sampler, start struggling with stream

7e3ff18

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Initialize stream after internal accessors

f77fc36

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Handle pointers

a8982bc

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Fix hp

e564a88

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

bader reviewed Apr 20, 2020

View reviewed changes

elizabethandrews reviewed Apr 20, 2020

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

Erich Keane and others added 4 commits April 21, 2020 09:22

Refactor constructKernelName to return a pair.

a964a25

Since the only caller to this now uses both values, refactor it to just return a pair and unpack on the other side. Signed-off-by: Erich Keane <erich.keane@intel.com>

Simplify run line of the test

8568786

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Apply CR comments

f4ab328

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Apply clang-format

645774b

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Fznamznon dismissed erichkeane’s stale review via f4ab328 April 21, 2020 09:40

bader previously approved these changes Apr 21, 2020

View reviewed changes

bader requested a review from erichkeane April 21, 2020 10:34

erichkeane previously approved these changes Apr 21, 2020

View reviewed changes

Try to fix LIT test on windows

b0a03e6

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Fznamznon dismissed stale reviews from erichkeane and bader via b0a03e6 April 22, 2020 09:10

Fznamznon requested review from bader and erichkeane April 22, 2020 11:07

bader approved these changes Apr 22, 2020

View reviewed changes

erichkeane approved these changes Apr 22, 2020

View reviewed changes

bader merged commit de1c363 into intel:sycl Apr 22, 2020

bader mentioned this pull request May 18, 2020

WIP on 'visitor' model for the SemaSYCL refactor #1495

Closed

premanandrao mentioned this pull request Mar 30, 2022

[SYCL] Refactor processing of parallel_for_work_group constructs #5918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] SemaSYCL significant refactoring #1517

[SYCL] SemaSYCL significant refactoring #1517

Fznamznon commented Apr 14, 2020 •

edited by erichkeane

Loading

erichkeane left a comment

Fznamznon commented Apr 16, 2020

erichkeane commented Apr 16, 2020

erichkeane commented Apr 20, 2020

elizabethandrews commented Apr 20, 2020

bader left a comment

bader Apr 20, 2020

erichkeane Apr 20, 2020

Fznamznon Apr 21, 2020

erichkeane Apr 21, 2020

elizabethandrews left a comment

erichkeane left a comment

Fznamznon commented Apr 22, 2020

erichkeane commented Apr 22, 2020

bader commented Apr 22, 2020

erichkeane commented Apr 22, 2020

bader commented Apr 22, 2020

		@@ -1,4 +1,4 @@
		// RUN: %clang -I %S/Inputs -fsycl-device-only -Xclang -fsycl-int-header=%t.h %s -c -o %T/kernel.spv
		// RUN: %clang -I %S/Inputs -fsycl-device-only -Xclang -fsycl-int-header=%t.h %s -fsyntax-only -o %T/kernel.spv

[SYCL] SemaSYCL significant refactoring #1517

[SYCL] SemaSYCL significant refactoring #1517

Conversation

Fznamznon commented Apr 14, 2020 • edited by erichkeane Loading

erichkeane left a comment

Choose a reason for hiding this comment

Fznamznon commented Apr 16, 2020

erichkeane commented Apr 16, 2020

erichkeane commented Apr 20, 2020

elizabethandrews commented Apr 20, 2020

bader left a comment

Choose a reason for hiding this comment

bader Apr 20, 2020

Choose a reason for hiding this comment

erichkeane Apr 20, 2020

Choose a reason for hiding this comment

Fznamznon Apr 21, 2020

Choose a reason for hiding this comment

erichkeane Apr 21, 2020

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

Fznamznon commented Apr 22, 2020

erichkeane commented Apr 22, 2020

bader commented Apr 22, 2020

erichkeane commented Apr 22, 2020

bader commented Apr 22, 2020

Fznamznon commented Apr 14, 2020 •

edited by erichkeane

Loading