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

erichkeane · 2020-04-08T15:38:52Z

This is my first run at how I think we can re-arch this to better share work. Suggestions/patches welcome :)

I've implemented ONLY the arg-type diagnostics so far, but I intend to start looking at other parts when I get a chance.

Signed-off-by: Erich Keane [email protected]

Signed-off-by: Erich Keane <[email protected]>

erichkeane · 2020-04-08T16:36:27Z

clang/lib/Sema/SemaSYCL.cpp

+    else if (ArgTy->isScalarType())
+      KF_FOR_EACH(handleScalarType);
+    else
+      llvm_unreachable("Unsupported kernel parameter type");


This almost definitely shouldn't be here. I should add a "handleOtherType" that is only implemented by the diagnostics, which simply diagnoses with an error. Everyone else can ignore it.

MrSidims · 2020-04-08T16:53:36Z

Just adding myself to reviewers to not miss what is going on.

Fznamznon · 2020-04-08T15:59:03Z

clang/lib/Sema/SemaSYCL.cpp

+  void handleSyclAccessorType(const FieldDecl *, QualType){}
+  void handleSyclSamplerType(const FieldDecl *, QualType){}


I think handling for accessor and sampler types will be identical. Do we really need to split it?

I could swear I saw a spot where we handled them separately (but I don't have the terminal open, but I want to say the Integration Header?). I would think we can keep them split for the near future, and combine them if we find they aren't particularly useful to be separate.

Ah, right. The handling in integration header can be different for accessors and samplers, but It should be identical for generation of OpenCL kernel body.

Ok, that matches with my understanding as well! I presume the OpenCL Kernel body/decl will likely implement theirs in a function that this just calls. Maybe I'll make that part a bit easier when I get there.

Fznamznon · 2020-04-08T16:58:54Z

clang/lib/Sema/SemaSYCL.cpp

+};
+
+// A type to Create and own the FunctionDecl for the kernel.
+class SYCLKernelHeader : public SyclKernelFieldHandler<SYCLKernelHeader> {


Why this class is needed at all?
What is kernel obj header?

I'm not sure yet :) I was brainstorming based on the current things we do. The idea was to have this + the error-checking version replace BuildArgTys. This is more just to build/own the declaration and much of the stuff that happens in ConstructOpenCLKernel.

erichkeane · 2020-04-08T17:03:23Z

clang/lib/Sema/SemaSYCL.cpp

+// A base type that the SYCL OpenCL Kernel construction task uses to implement
+// individual tasks.
+template<typename Derived>
+class SyclKernelFieldHandler {


We also need one of these for the integration header generation.

erichkeane · 2020-04-08T17:04:31Z

clang/lib/Sema/SemaSYCL.cpp

+  void handleSyclAccessorType(const FieldDecl *, QualType){}
+  void handleSyclSamplerType(const FieldDecl *, QualType){}


I could swear I saw a spot where we handled them separately (but I don't have the terminal open, but I want to say the Integration Header?). I would think we can keep them split for the near future, and combine them if we find they aren't particularly useful to be separate.

…or. Still need IntHeader and kernel body generation Signed-off-by: Erich Keane <[email protected]>

erichkeane · 2020-04-10T01:55:03Z

Did a pretty sizable amount of work this evening, and go tmyself to the point where I think I have a pretty good framework. I still don't have the body of the SYCL Kernel being generated, but I think I've replaced all of buildArgTys. I also added the type for Integration Headers, but didn't do any implementation for it.

Signed-off-by: Erich Keane <[email protected]>

Fznamznon

Okay, it evolves.
I can take a look into kernel body generation.

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-04-10T08:19:00Z

clang/lib/Sema/SemaSYCL.cpp

+  }
+
+  void handlePointerType(const FieldDecl *FD, QualType ArgTy) final {
+    // TODO: Can we document what the heck this is doing?!


Yep, that would be nice. But it's not so hard to understand what is happening here looking into the code.
This is just handling of USM pointers. I haven't seen any doc about implementation details of USM, only specs (https://github.com/intel/llvm/tree/sycl/sycl/doc/extensions/USM).
Basically USM allows to use raw pointers instead of buffers/accessors, but these pointers point to the specially allocated memory. For any pointer we add some argument with the same type but global address space, because OpenCL requires it.
The problem here that we don't know how to differentiate USM pointers from any others, so we stupidly consider them all as USM pointers. This can lead to some problems, I guess.

I've read both the code and your post and don't have a good hold of it other than it seems to be just creating a pointer to the same type, except global. Patches that do a good job explaining the details would be welcome :)

Hi. Sorry.

At least when I did this, kernel args to OpenCL that are pointers had to be in the global address space.

I make no claims to having done the right thing, only that I did something that got pointers to work.

And yes - I don't see a way to disambiguate 'normal' C++ pointers from USM pointers since USM is very specifically trying to avoid modifying the type system.
Previously, the compiler was rejecting all programs with pointers, so things that should work wouldn't.
We had to make it so any pointer works, which means that legal things now work, but illegal things will crash. I still view that as an improvement.

Fznamznon · 2020-04-10T08:21:03Z

clang/lib/Sema/SemaSYCL.cpp


 // Generates the OpenCL kernel using KernelCallerFunc (kernel caller
-// function) defined is SYCL headers.
+// function) defined is Sycl headers.


I was being incredibly inconsistent with my naming at one point, so I ran a find/replace. I guess it got a little out of hand :)

Fznamznon · 2020-04-10T08:29:53Z

clang/lib/Sema/SemaSYCL.cpp

+  static FunctionDecl *initKernelObj(ASTContext &Ctx, StringRef Name,
+                                     SourceLocation Loc, bool IsInline) {


I don't get why it is called initKernelObj. Shouldn't it be something like createKernelDecl?
BTW, I don't think that OpenCL kernel somewhat can be inline. o_O

The implicit inline logic came from the existing implementation.

Naming things is hard... 'initKernelObj' (with the variable named KernelObj) made sense at the time shrug.

I think because the name KernelObj is incorrect.

FunctionDecl *KernelObj;

I think this variable contains declaration (and definition after we generate the body) of OpenCL kernel. We have to rename it so it will be easy to understand that this is OpenCL kernel function declaration. Something like KernelDecl.

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-04-10T08:37:21Z

clang/lib/Sema/SemaSYCL.cpp

+// A type that handles the accessor recursion and acts as a base for
+// SyclKernelDeclCreator. It doesn't 'own' anything other than the KernelObj
+// pointer and functionality required to add a param.
+class SyclKernelDeclBase : public SyclKernelFieldHandler<SyclKernelDeclBase> {


I don't really get why do we need a base and a child class for kernel decl creation.
Could you please elaborate a bit more?

The thought is there are 2 extremely similar cases: 1- When dealing with the 'root' of the call tree, and 2- when recursing down a 'struct'.

The struct only gets 2 of the cases (recursing down the struct plus accessors). However, because the two cases are so extremely similar (and the 'base' needs just about all the fields/abilities anyway), it seemed to make sense to make it a base class.

I'd love to see another suggestion here, I wanted to maximize code reuse without having a bunch of free-functions that seemed to live outside the visitor, but I didn't immediately come up with a better one.

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-04-10T13:07:28Z

I'll do a bunch of the renaming/changes that @Fznamznon suggested on this patch.

Signed-off-by: Erich Keane <[email protected]>

…e :)

Signed-off-by: Erich Keane <[email protected]>

…ill need to fix the integration header offset calculation/keeping track Signed-off-by: Erich Keane <[email protected]>

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

erichkeane · 2020-04-10T22:06:19Z

Ok, pushed a couple more commits. These changed the look of the visitor slightly to handle recursion and base types.

Additionally, it SHOULD properly handle accessors as both fields and base types all the way up the tree.

We need tests for that, there are quite a few TODOs, we still need the body implementation, it would be nice to get Elizabeth's implementation of memcpy working alright (perhaps after body + a squash?), and it would be nice to get arrays working as well.

For now, I'm just quitting for the weekend, I'll pick this up on monday.

Signed-off-by: Erich Keane <[email protected]>

…y creator to get the list of parameters for the currently handled field Signed-off-by: Erich Keane <[email protected]>

rdeodhar · 2020-04-21T23:55:14Z

clang/lib/Sema/SemaSYCL.cpp

+    else if (Util::isSyclStreamType(ItemTy))
+      (void)std::initializer_list<int>{
+          (handlers.handleSyclStreamType(Item, ItemTy), 0)...};
+    else if (ItemTy->isStructureOrClassType()) {


Here you need the following, otherwise an array of accessors within a struct do not get processed.

else if (ItemTy->isArrayType()) (void)std::initializer_list<int>{ (handlers.handleArrayType(Item, ItemTy), 0)...};

Hmm... I didn't see in the old code it recursing through arrays, are we intending to split them out as well?

In that case, we probably need to better handle the offsets.

Issue captured here: #1567

It is quite natural to recurse through structs to any level to discover accessors and the new visitor model does that.
The existing implementation does not deal with arrays at all so you won't see any code there.
My support for arrays accounted for arrays of accessors, i.e., the array type is one of the special sycl types. When such an array type was encountered it would "unroll" each element of the array and generate a separate descriptor for it. It did not try to handle arrays of other struct types that contained accessors (or arrays of accessors) as members, or more deeply nested members.
I suggest we support arrays of accessors only, not arrays of structs that may in turn contain accessors.
If so, the proposed addition to the visitor function will do the job.

I think we'd have to support both, arrays of structs of accessors, or even arrays of structs of arrays of accessors.

It is a presumably fairly easy addition however, but I'll hold it off for another patch, we have a fairly sizable change coming from @elizabethandrews on how parameters are passed, so I'd like to hold off on arrays until after that.

rdeodhar · 2020-04-21T23:59:05Z

clang/lib/Sema/SemaSYCL.cpp

+  // class/field graph. Int Headers use this to calculate offset, most others
+  // don't have a need for these.
+
+  virtual void enterStruct(const CXXRecordDecl *, const FieldDecl *) {}


I don't know if leaveStruct will ever be needed. The offsets generated for the integration header are monotonically increasing. If the lambda object members are visited in ascending sequence, the offset value should be steadily increasing, thus it will never need bumping down.

I confess I don't fully understand the base class stuff either. The old code made no reference to base classes.

I did the 'leaveStruct' so that we can 'undo' the offset, that way we don't have to store the entire 'stack' of parents. Instead, we can store just a single offset, and add/subtract it as needed. For example, if we are deep in a set of structures and see:

struct S { accessor a; }; struct U { /*other fields;*/S s1, s2; };

We would have to 'know' about U and the entire set of parents in order to calculate the offset. By doing the 'leaveStruct' (see the Int Header implementation), each time you enter a field, you simply increase by your offset, then decrease by the same offset when we leave. So in the above:

We're in 'U', offset is something like 95 (random number).
We enter 's1', which we know is at offset '40' in 'U', so we increase our offset by that much.
We leave 's1', we can calculate the offset as '40' in 'U' and just decrease it, getting us back to 95.
Then, we enter 's2', we can do the same.

Otherwise, we'd have to know 'U''s offset, and our current field/bases' offset as well. I couldn't come up with a way of doing that which didn't store the stack of currently visited things.

bader · 2020-05-18T16:37:19Z

Should we close this PR?
#1517 seems to replace this one.

WIP on 'visitor' model for the SemaSYCL refactor

9cd5f70

Signed-off-by: Erich Keane <[email protected]>

erichkeane requested review from Fznamznon, kbobrovs and elizabethandrews April 8, 2020 15:38

erichkeane mentioned this pull request Apr 8, 2020

OpenCL Kernel Generation in SemaSYCL.cpp needs significant refactoring- #1491

Closed

clang format

007b753

Signed-off-by: Erich Keane <[email protected]>

erichkeane commented Apr 8, 2020

View reviewed changes

MrSidims self-requested a review April 8, 2020 16:53

Fznamznon reviewed Apr 8, 2020

View reviewed changes

erichkeane commented Apr 8, 2020

View reviewed changes

elizabethandrews requested a review from rdeodhar April 9, 2020 17:21

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

6f8e747

…or. Still need IntHeader and kernel body generation Signed-off-by: Erich Keane <[email protected]>

Erich Keane added 2 commits April 9, 2020 18:57

clang-format changes

4d37b4c

Signed-off-by: Erich Keane <[email protected]>

more clang-format changes

01da1e7

Signed-off-by: Erich Keane <[email protected]>

cperkinsintel self-requested a review April 10, 2020 02:15

Fznamznon reviewed Apr 10, 2020

View reviewed changes

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

Erich Keane added 7 commits April 10, 2020 06:37

A few spelling changes, move the kernel type changing

e5ed20c

Signed-off-by: Erich Keane <[email protected]>

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

f972476

…e :)

WIP with integration header

9cb607e

Signed-off-by: Erich Keane <[email protected]>

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

39e988c

…ill need to fix the integration header offset calculation/keeping track Signed-off-by: Erich Keane <[email protected]>

small refactoring for base classes

055fcd4

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

936f36f

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

128f7b1

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

Erich Keane added 2 commits April 10, 2020 15:07

Format the createOpenCLKernel func, remove int header from comment

645c513

Rename kernelobj to kerneldecl

7682da8

Signed-off-by: Erich Keane <[email protected]>

Erich Keane added 3 commits April 13, 2020 08:28

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

7735dc8

Signed-off-by: Erich Keane <[email protected]>

Fix all the build errors from the last patch

a779ab2

Signed-off-by: Erich Keane <[email protected]>

Add getParamVarDeclsForCurrentField, which provides a way for the bod…

30d446b

…y creator to get the list of parameters for the currently handled field Signed-off-by: Erich Keane <[email protected]>

Fznamznon mentioned this pull request Apr 14, 2020

[SYCL] SemaSYCL significant refactoring #1517

Merged

rdeodhar reviewed Apr 21, 2020

View reviewed changes

erichkeane closed this May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

erichkeane commented Apr 8, 2020

erichkeane Apr 8, 2020

MrSidims commented Apr 8, 2020

Fznamznon Apr 8, 2020

erichkeane Apr 8, 2020

Fznamznon Apr 8, 2020

erichkeane Apr 8, 2020

Fznamznon Apr 8, 2020

erichkeane Apr 8, 2020

erichkeane Apr 8, 2020

erichkeane Apr 8, 2020

erichkeane commented Apr 10, 2020

Fznamznon left a comment

Fznamznon Apr 10, 2020

erichkeane Apr 10, 2020

jbrodman Apr 11, 2020

Fznamznon Apr 10, 2020

erichkeane Apr 10, 2020

Fznamznon Apr 10, 2020

erichkeane Apr 10, 2020

Fznamznon Apr 13, 2020

Fznamznon Apr 10, 2020

erichkeane Apr 10, 2020

erichkeane commented Apr 10, 2020

erichkeane commented Apr 10, 2020

rdeodhar Apr 21, 2020

erichkeane Apr 22, 2020

erichkeane Apr 22, 2020

rdeodhar Apr 22, 2020

erichkeane Apr 22, 2020

rdeodhar Apr 21, 2020

erichkeane Apr 22, 2020

bader commented May 18, 2020

		void handleSyclAccessorType(const FieldDecl *, QualType){}
		void handleSyclSamplerType(const FieldDecl *, QualType){}

		static FunctionDecl *initKernelObj(ASTContext &Ctx, StringRef Name,
		SourceLocation Loc, bool IsInline) {

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

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

Conversation

erichkeane commented Apr 8, 2020

Choose a reason for hiding this comment

MrSidims commented Apr 8, 2020

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

erichkeane commented Apr 10, 2020

Fznamznon 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

erichkeane commented Apr 10, 2020

erichkeane commented Apr 10, 2020

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

bader commented May 18, 2020