[SYCL] Adding support for 'sycl_special_class' attribute. #3892

zahiraam · 2021-06-05T17:39:00Z

Special classes such as accessor, sampler, and stream need additional implementation when they are passed from host to device. Currently the compiler recognizes these classes by their name.
This patch is adding a new attribute “sycl_special_class” used to mark SYCL classes that need the additional compiler handling.
Usage:

class __attribute__((sycl_special_class)) class  special_class {
...
};

This attribute is used in SYCL headers to mark SYCL classes which need additional compiler handling when passed from host to device. Attribute can be applied to struct/class and can have optional argument which indicates kind of SYCL special class, so it can be used to implement handling of some generic case of SYCL special class as well as implement different handling for each kind of SYCL special class. Usage: ``` class __attribute__((sycl_special_class(accessor))) accessor { ... } ```

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

zahiraam · 2021-06-15T19:39:58Z

@Fznamznon
I have incorporated the code from #2091

There are still some issues. I am trying to compile this test case:

#include "sycl.hpp"

sycl::queue myQueue;

class __attribute__((sycl_special_class(accessor))) AccessorBase {
  int A;
public:
  sycl::accessor<int, 1, sycl::access::mode::read_write,
                 sycl::access::target::local>
  acc;
};

int main() {

  AccessorBase AS;
  myQueue.submit([&](sycl::handler &h) {
    h.single_task<class XYZ>([=]() {
      AS.acc.use();

    });
  });

  return 0;
}

First is this a legal test case? If it is, I am expecting that the __init method be created. But it crashing with an assert at handleSpecialType because InitMethod is null.

The same test using class AccessorBase instead, is creating the __init method. As far as I can tell, it is happening during the call to visitRecord. With the attribute sycl_special_class(accessor) , the call to the function visitRecord doesn’t happen before handleSpecialType. And this function is using the InitMethod. What am I missing? Should handleSpecialType call visitRecord?
Any comments/suggestions are welcome.

zahiraam · 2021-06-16T12:59:51Z

@erichkeane Would you mind taking a loot at this and give me some feedback. I have added the attribute, but getting a crash in the handleSpecialType because the __init function is not created. Can you please let me know if the test case I am using (comment above) is actually correct and what I am missing? thanks.

Fznamznon · 2021-06-16T13:07:38Z

Please keep in mind the idea described by Victor here #2091 (comment) , so the original patch needs rework.

First is this a legal test case? If it is, I am expecting that the __init method be created. But it crashing with an assert at handleSpecialType because InitMethod is null.

It should be defined by the class that have sycl_special_class attribute, so it seems the test case you mentioned is not completely valid. FE job here is only generate a call to __init method.

erichkeane · 2021-06-16T13:13:27Z

Please keep in mind the idea described by Victor here #2091 (comment) , so the original patch needs rework.

First is this a legal test case? If it is, I am expecting that the __init method be created. But it crashing with an assert at handleSpecialType because InitMethod is null.

It should be defined by the class that have sycl_special_class attribute, so it seems the test case you mentioned is not completely valid. FE job here is only generate a call to __init method.

Yep, exactly this. Any types with this attribute need to have their __init method DEFINED. The attribute then is supposed to cause the SemaSYCL visitor handling to emit a call to said __init method.

zahiraam · 2021-06-16T13:24:46Z

@erichkeane Ok thanks.
You say "Any types with this attribute need to have their __init method DEFINED"
Where does this happen? Still not quite clear about this. Should I have a function instantiateSYCLSpecialClassAttr function?

erichkeane · 2021-06-16T13:26:29Z

@erichkeane Ok thanks.
You say "Any types with this attribute need to have their __init method DEFINED"
Where does this happen? Still not quite clear about this. Should I have a function instantiateSYCLSpecialClassAttr function?

In the source code? Check out how accessor is implemented today. It needs to still have that __init method.

The point of this patch is for the library code to still be more or less like it is now, except with the added attribute so that we're not detecting these 'special' types directly by name.

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

smanna12

@zahiraam, you need to add // clang-format on line1597 instead of line#1588

elizabethandrews · 2021-10-14T19:19:23Z

@zahiraam, you need to add // clang-format on line1597 instead of line#1588

@smanna12 is this the norm for clang-format fails? As in, are we expected to add these comments?

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

smanna12 · 2021-10-14T19:24:20Z

@zahiraam, you need to add // clang-format on line1597 instead of line#1588

@smanna12 is this the norm for clang-format fails? As in, are we expected to add these comments?

I remember that i did this on one of my PR #2399 suggested by reviewer. I am not sure whether this is the norm for clang-format fails. Tagging @bader for this specific question?

keryell · 2021-10-14T20:53:49Z

I remember that i did this on one of my PR #2399 suggested by reviewer. I am not sure whether this is the norm for clang-format fails. Tagging @bader for this specific question?

Interesting. Perhaps this is solved by using latest clang-format-14 from HEAD?

zahiraam · 2021-10-14T21:27:13Z

I remember that i did this on one of my PR #2399 suggested by reviewer. I am not sure whether this is the norm for clang-format fails. Tagging @bader for this specific question?

Interesting. Perhaps this is solved by using latest clang-format-14 from HEAD?

@keryell Yes that's the version I am using!

bader · 2021-10-15T07:40:05Z

@zahiraam, you need to add // clang-format on line1597 instead of line#1588

@smanna12 is this the norm for clang-format fails? As in, are we expected to add these comments?

I remember that i did this on one of my PR #2399 suggested by reviewer. I am not sure whether this is the norm for clang-format fails. Tagging @bader for this specific question?

I don't think we should apply suggestion for the test from #2399 for DCP++ runtime headers. @intel/llvm-reviewers-runtime, does this suggestion works for you?

romanovvlad · 2021-10-15T11:59:06Z

@zahiraam, you need to add // clang-format on line1597 instead of line#1588

@smanna12 is this the norm for clang-format fails? As in, are we expected to add these comments?

I remember that i did this on one of my PR #2399 suggested by reviewer. I am not sure whether this is the norm for clang-format fails. Tagging @bader for this specific question?

I don't think we should apply suggestion for the test from #2399 for DCP++ runtime headers. @intel/llvm-reviewers-runtime, does this suggestion works for you?

Do not think we should do it for runtime headers either.

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

zahiraam · 2021-10-15T12:16:33Z

@zahiraam, you need to add // clang-format on line1597 instead of line#1588

@smanna12 is this the norm for clang-format fails? As in, are we expected to add these comments?

I remember that i did this on one of my PR #2399 suggested by reviewer. I am not sure whether this is the norm for clang-format fails. Tagging @bader for this specific question?

I don't think we should apply suggestion for the test from #2399 for DCP++ runtime headers. @intel/llvm-reviewers-runtime, does this suggestion works for you?

Do not think we should do it for runtime headers either.

Removing the clang-format directives and fixing lines 1575 didn't work. I am going for the more aggressive indentation fix even if it looks like an "unrelated" change. clang-format thinks that it is!

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

bader · 2021-10-15T12:25:03Z

@zahiraam, you need to add // clang-format on line1597 instead of line#1588

@smanna12 is this the norm for clang-format fails? As in, are we expected to add these comments?

I remember that i did this on one of my PR #2399 suggested by reviewer. I am not sure whether this is the norm for clang-format fails. Tagging @bader for this specific question?

I don't think we should apply suggestion for the test from #2399 for DCP++ runtime headers. @intel/llvm-reviewers-runtime, does this suggestion works for you?

Do not think we should do it for runtime headers either.

Removing the clang-format directives and fixing lines 1575 didn't work. I am going for the more aggressive indentation fix even if it looks like an "unrelated" change. clang-format thinks that it is!

The suggested change doesn't look correct to me. It seems that ifdef directive confuses clang-format. I suggest we discuss this issue in separate PR as it's not related here. I'm okay to ignore clang-format failure.

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

elizabethandrews · 2021-10-15T14:48:14Z

Actually, can you update your PR description - that will be the commit message. It is currently outdated.

bader · 2021-10-19T10:22:59Z

@againull, @intel/llvm-reviewers-runtime, ping.

Fznamznon and others added 14 commits July 10, 2020 18:22

Merge remote-tracking branch 'remote/sycl' into pr_2091

5e679f8

Fix build issues

bdb1ff5

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Merge remote-tracking branch 'remote/sycl' into pr_2091

203edd6

Fixing build errors

dc74a5e

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Indent

65ac140

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Indent

6586846

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Fixed build errors

a3c118b

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Merge remote-tracking branch 'remote/sycl' into pr_2091

30c5a03

Merge remote-tracking branch 'remote/sycl' into pr_2091

3999939

Fix LIT tests

c3f8757

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Merge remote-tracking branch 'remote/sycl' into pr_2091

ab4c68e

Fixed build errors

58db6b5

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Merge remote-tracking branch 'remote/sycl' into pr_2091

c009836

Fixed build errors

05b0af8

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

zahiraam changed the title ~~Pr 2091~~ [SYCL] Adding support for sycl_special_class attribute. Jun 17, 2021

zahiraam changed the title ~~[SYCL] Adding support for sycl_special_class attribute.~~ [SYCL] Adding support for 'sycl_special_class' attribute. Jun 17, 2021

zahiraam added 2 commits June 18, 2021 09:51

Fixed build errors

425bb97

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Merge remote-tracking branch 'remote/sycl' into pr_2091

890e4b0

zahiraam marked this pull request as ready for review June 18, 2021 17:24

zahiraam requested review from AaronBallman, againull, elizabethandrews and premanandrao as code owners June 18, 2021 17:24

smanna12 reviewed Oct 14, 2021

View reviewed changes

Fix formatting

462ff09

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

elizabethandrews previously approved these changes Oct 14, 2021

View reviewed changes

Removed clang-format directives and fixed indentation

d170717

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

zahiraam dismissed elizabethandrews’s stale review via d170717 October 15, 2021 12:04

Fix indentation

80cb0d6

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

Fix indentation

1aa856c

Signed-off-by: Zahira Ammarguellat <zahira.ammarguellat@intel.com>

elizabethandrews approved these changes Oct 15, 2021

View reviewed changes

zahiraam requested review from elizabethandrews and smanna12 October 15, 2021 21:16

smanna12 approved these changes Oct 18, 2021

View reviewed changes

elizabethandrews approved these changes Oct 18, 2021

View reviewed changes

againull approved these changes Oct 20, 2021

View reviewed changes

bader merged commit 7187f7b into intel:sycl Oct 21, 2021

elizabethandrews mentioned this pull request Nov 15, 2021

[WIP][SYCL] Implement sycl_special_class attribute #2091

Closed

srividya-sundaram mentioned this pull request Dec 10, 2021

[SYCL] Add new kernel-arg-runtime-aligned metadata. #5111

Merged

elizabethandrews mentioned this pull request Dec 20, 2021

[ESIMD] Fix sampler/stream parameter handling for ESIMD kernels #5165

Merged

Fznamznon mentioned this pull request Jan 31, 2022

[SYCL][FE] Add an attribute to annotate special sycl types #2041

Closed

elizabethandrews mentioned this pull request May 10, 2022

[SYCL] Refactor accessor handling #4115

Closed

zahiraam mentioned this pull request Jul 13, 2023

Remove FE dependency on SYCL header scope of accessor properties #3529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Adding support for 'sycl_special_class' attribute. #3892

[SYCL] Adding support for 'sycl_special_class' attribute. #3892

zahiraam commented Jun 5, 2021 •

edited by bader

Loading

zahiraam commented Jun 15, 2021 •

edited by bader

Loading

zahiraam commented Jun 16, 2021

Fznamznon commented Jun 16, 2021

erichkeane commented Jun 16, 2021

zahiraam commented Jun 16, 2021 •

edited

Loading

erichkeane commented Jun 16, 2021

smanna12 left a comment •

edited

Loading

elizabethandrews commented Oct 14, 2021

smanna12 commented Oct 14, 2021 •

edited

Loading

keryell commented Oct 14, 2021

zahiraam commented Oct 14, 2021

bader commented Oct 15, 2021

romanovvlad commented Oct 15, 2021

zahiraam commented Oct 15, 2021

bader commented Oct 15, 2021

elizabethandrews commented Oct 15, 2021

bader commented Oct 19, 2021

[SYCL] Adding support for 'sycl_special_class' attribute. #3892

[SYCL] Adding support for 'sycl_special_class' attribute. #3892

Conversation

zahiraam commented Jun 5, 2021 • edited by bader Loading

zahiraam commented Jun 15, 2021 • edited by bader Loading

zahiraam commented Jun 16, 2021

Fznamznon commented Jun 16, 2021

erichkeane commented Jun 16, 2021

zahiraam commented Jun 16, 2021 • edited Loading

erichkeane commented Jun 16, 2021

smanna12 left a comment • edited Loading

Choose a reason for hiding this comment

elizabethandrews commented Oct 14, 2021

smanna12 commented Oct 14, 2021 • edited Loading

keryell commented Oct 14, 2021

zahiraam commented Oct 14, 2021

bader commented Oct 15, 2021

romanovvlad commented Oct 15, 2021

zahiraam commented Oct 15, 2021

bader commented Oct 15, 2021

elizabethandrews commented Oct 15, 2021

bader commented Oct 19, 2021

zahiraam commented Jun 5, 2021 •

edited by bader

Loading

zahiraam commented Jun 15, 2021 •

edited by bader

Loading

zahiraam commented Jun 16, 2021 •

edited

Loading

smanna12 left a comment •

edited

Loading

smanna12 commented Oct 14, 2021 •

edited

Loading