[SYCL] Adjust kernel parameters requirements #1014

Fznamznon · 2020-01-15T14:03:32Z

Standard layout is too restrictive (prevents things like tuple). Enable standard layout requirement only if -sycl-std=1.2.1 is set.
To make using of non-standard layout structs safe compiler builds structs layout in accordance with host ABI.
Add trivially copyable requirement for all parameters.

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

bader

LGTM with a few suggestions.

bader · 2020-01-15T14:20:37Z

clang/lib/AST/RecordLayoutBuilder.cpp

@@ -2199,8 +2199,9 @@ static bool mustSkipTailPadding(TargetCXXABI ABI, const CXXRecordDecl *RD) {
  llvm_unreachable("bad tail-padding use kind");
 }

-static bool isMsLayout(const ASTContext &Context) {
-  return Context.getTargetInfo().getCXXABI().isMicrosoft();
+static bool isMsLayout(const ASTContext &Context, bool CheckAuxABI = false) {


What if we change parameter type to TargetInfo?

Suggested change

static bool isMsLayout(const ASTContext &Context, bool CheckAuxABI = false) {

static bool isMsLayout(const TargeInfo *TI) {

usage

if (isMsLayout(*this.getTargetInfo())) ... ... if (isMsLayout(*this.getAuxTargetInfo())) ...

Then we will need to change usage of this function for 3 additional times not including the one which I changed. And these 3 additional places of isMsLayout usage aren't connected with my current patch. Are you sure?

I don't see any problems with this refactoring.
The benefit I see from this change is that we move if statement higher in the call stack as exactly the same logic is already implemented higher in the calls stack. On the other hand, I hope all isMsLayout calls are inlined and this might not be a problem.
I'll leave final decision to you.

I'd like to keep unrelated parts of code unchanged.

clang/test/CodeGenSYCL/non-standard-layout.cpp

clang/test/SemaSYCL/non-trivially-copyable-kernel-param.cpp

clang/test/SemaSYCL/reference-kernel-param.cpp

Ruyk · 2020-01-15T15:18:33Z

Is that now the default to accept trivially copyable instead of standard layout? The current SYCL 1.2.1 specification still mentions standard layout. I think maybe this should be an option when you pass "-std=sycl-1.2.1-intel" so that "-std=sycl-1.2.1" is conformant to the current spec.

bader · 2020-01-15T15:57:13Z

Is that now the default to accept trivially copyable instead of standard layout? The current SYCL 1.2.1 specification still mentions standard layout. I think maybe this should be an option when you pass "-std=sycl-1.2.1-intel" so that "-std=sycl-1.2.1" is conformant to the current spec.

Make sense to me.
I would start with a separate front-end option (e.g. -fsycl-new-kernel-param-requirements), which -std=sycl-1.2.1 driver option turns off.
NOTE: -std option doesn't support sycl-1.2.1-intel value.

rolandschulz · 2020-01-15T18:53:04Z

Note that the standard currently requires standard layout and trivially copyable. Thus if we make the check for std-layout a compiler option than the more strict option should check for both and not just standard layout (as before this change).

BTW: the standard doesn't require a diagnostic from the compiler. It just says it is a requirement without saying whether it is UB/implementation-defined/diagnostic-required/.... Thus I don't think removing the check makes it non-spec compliant. Having said that I'm not apposed to add a compiler switch.

Ruyk · 2020-01-16T12:50:24Z

I am not entirely disagreeing with you, but the spec document only requires standard layout for kernel parameter passing.

SYCL 1.2.1, 3.10:

Sharing data structures between host and device code imposes certain restrictions, such as use of only user defined classes that are C++11 standard layout classes for the data structures, and in general, no pointers initialized for the host can be used on the device.

SYCL 1.2.1, 4.8.9.1:

A kernel can be defined as a named function object type. These function objects provide the same functionality as any C++ function object, with the restriction that they need to follow C++11 standard layout rules

SYCL 1.2.1, 4.8.11: (Rules for kernel parameters)

C++ standard layout values must be passed by value to the kernel.
C++ non-standard layout values must not be passed as arguments to a kernel that is compiled for a device.

Trivially copyable is a condition only for the T in buffer<T>, See SYCL 1.2.1, 4.7.2:

More generally, since the value type of a buffer is required to be trivially copyable, there is no constructor or destructor called in any case.

The only mention of both trivially copyable and standard layout is on Table 4.78 for the set_args interoperability function.

Fznamznon · 2020-01-17T14:19:51Z

Okay, for me the SPEC seems a bit unclear about trivially-copyable requirement, when it's clear about standard layout requirement, so I've added a front-end option, which is not enabled when -sycl-std=1.2.1 is passed to driver. Note that we always pass some -sycl-std=1.2.1 option to front-end.

clang/include/clang/Basic/LangOptions.def

clang/test/SemaSYCL/non-trivially-copyable-kernel-param.cpp

clang/test/SemaSYCL/reference-kernel-param.cpp

bader

LGTM.

clang/test/SemaSYCL/non-std-layout-param.cpp

rolandschulz · 2020-01-17T23:59:13Z

SYCL 1.2.1, 3.10:

Sharing data structures between host and device code imposes certain restrictions, such as use of only user defined classes that are C++11 standard layout classes for the data structures, and in general, no pointers initialized for the host can be used on the device.

The way this is written this refers to both kernel arguments and buffers AFAICS. The sentence before even explicitly lists both lambda capture and accessors. Of course I suggest we remove the whole (IMO useless) std-layout requirement from the spec.

Trivially copyable is a condition only for the T in buffer

You are right. But this is a clear omission and needs to be fixed. There is no way to implement argument passing without breaking the C++ memory model (http://eel.is/c++draft/basic.types#3).

rolandschulz · 2020-01-18T00:02:57Z

clang/include/clang/Basic/LangOptions.def

@@ -233,6 +233,7 @@ LANGOPT(GPUMaxThreadsPerBlock, 32, 256, "default max threads per block for kerne
 LANGOPT(SYCLIsDevice      , 1, 0, "Generate code for SYCL device")
 LANGOPT(SYCLIsHost        , 1, 0, "SYCL host compilation")
 LANGOPT(SYCLAllowFuncPtr  , 1, 0, "Allow function pointers in SYCL device code")
+LANGOPT(SYCLNewKernelParamReq, 1, 0, "Enable trivially copyable requirement instead of standard layout requirement for SYCL kernel parameters")


I disagree with this choice. Trvially copyable should always be required. Even before we fix the SYCL spec it is required by the C++ spec. This option should only enable/disable whether std-layout is also checked. And the same way as our other usability improvements the default should be that the std-layout check is off.

So, we will always require trivially copyable and enable/disable standard layout using option.
@Ruyk , What do you think about it?

Yeah that makes some sense. The SYCL standard requires standard layout, which is why i would expect -sycl-std=sycl-1.2.1 to ask for standard layout.

@Ruyk, do you have any preference how non-standard extensions should be controlled by users?
I've created an issue here: #806 and I think it would be great to have a consistent approach across SYCL implementation.

requirement Standard layout is too restrictive (prevents things like tuple). To make using of non-standard layout structs safe compiler builds structs layout in accordance with host ABI. Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

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

Enable standard layout requirement passing front-end option which `-sycl-std=1.2.1.` driver option turnes on. Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

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

Fznamznon · 2020-01-21T09:33:51Z

Okay, I implemented checking of trivially copyable requirement by default and option to enable standard layout requirement checking. We check standard layout requirement only if -sycl-std=1.2.1 option is passed to the driver.
If someone is not agree with it, I can revert latest commit and return back to an option which switches between trivially copyable and standard layout requirements.

bader

Thanks! LGTM.

clang/include/clang/Driver/CC1Options.td

Ruyk · 2020-01-21T10:24:12Z

SYCL 1.2.1, 3.10:

Sharing data structures between host and device code imposes certain restrictions, such as use of only user defined classes that are C++11 standard layout classes for the data structures, and in general, no pointers initialized for the host can be used on the device.

The way this is written this refers to both kernel arguments and buffers AFAICS. The sentence before even explicitly lists both lambda capture and accessors. Of course I suggest we remove the whole (IMO useless) std-layout requirement from the spec.

Trivially copyable is a condition only for the T in buffer

You are right. But this is a clear omission and needs to be fixed. There is no way to implement argument passing without breaking the C++ memory model (http://eel.is/c++draft/basic.types#3).

That is why I said at the start that I am not entirely disagreeing with your comments. However, the fact is the published 1.2.1 spec is what it is, and if a change is needed, the place is the SYCL WG in Khronos IP zone, or at least the public SYCL-DOC spec, so everyone can track and follow this discussion.

In the meantime, I suggest we should try to keep all implementations aligned as to what is a valid SYCL program conformant to the spec. Whatever defaults you guys want to have is your choice, but to me at least when using -sycl-std=1.2.1, it should match what the text says.

Fznamznon · 2020-01-22T15:34:05Z

@rolandschulz ping.

Fznamznon requested review from bader, rolandschulz, elizabethandrews and erichkeane January 15, 2020 14:04

bader reviewed Jan 15, 2020

View reviewed changes

Fznamznon force-pushed the private/mpodchis/non-standard-layout branch from 1003bcc to 7bf61f1 Compare January 17, 2020 14:13

bader reviewed Jan 17, 2020

View reviewed changes

clang/test/SemaSYCL/non-std-layout-param.cpp Show resolved Hide resolved

rolandschulz reviewed Jan 18, 2020

View reviewed changes

Fznamznon added 4 commits January 20, 2020 14:03

Apply code review comments

ab9dd08

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

Apply comments

e002ee0

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

Apply comment

d6830e2

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

Fznamznon force-pushed the private/mpodchis/non-standard-layout branch from 37fb682 to d6830e2 Compare January 20, 2020 14:36

Fznamznon added 2 commits January 21, 2020 09:22

[SYCL] Enable trivially copyable requirement by default

18a87c5

Enable standard layout requirement passing front-end option which `-sycl-std=1.2.1.` driver option turnes on. Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

fsycl_std_layout_requirement -> fsycl_std_layout_kernel_params

4cb2eca

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

bader reviewed Jan 21, 2020

View reviewed changes

clang/include/clang/Driver/CC1Options.td Outdated Show resolved Hide resolved

bader approved these changes Jan 21, 2020

View reviewed changes

bader changed the title ~~[SYCL] Replace standard layout requirement with trivially copyable requirement~~ [SYCL] Adjust kernel parameters requirements Jan 21, 2020

bader requested a review from rolandschulz January 21, 2020 17:10

bader merged commit 3adb4a5 into intel:sycl Jan 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Adjust kernel parameters requirements #1014

[SYCL] Adjust kernel parameters requirements #1014

Fznamznon commented Jan 15, 2020 •

edited by bader

Loading

bader left a comment

bader Jan 15, 2020

Fznamznon Jan 17, 2020 •

edited

Loading

bader Jan 17, 2020

Fznamznon Jan 17, 2020

Ruyk commented Jan 15, 2020

bader commented Jan 15, 2020

rolandschulz commented Jan 15, 2020 •

edited

Loading

Ruyk commented Jan 16, 2020

Fznamznon commented Jan 17, 2020

bader left a comment

rolandschulz commented Jan 17, 2020

rolandschulz Jan 18, 2020

Fznamznon Jan 20, 2020

Ruyk Jan 21, 2020

bader Jan 21, 2020

Fznamznon commented Jan 21, 2020

bader left a comment

Ruyk commented Jan 21, 2020

Fznamznon commented Jan 22, 2020

	static bool isMsLayout(const ASTContext &Context, bool CheckAuxABI = false) {
	static bool isMsLayout(const TargeInfo *TI) {

[SYCL] Adjust kernel parameters requirements #1014

[SYCL] Adjust kernel parameters requirements #1014

Conversation

Fznamznon commented Jan 15, 2020 • edited by bader Loading

bader left a comment

Choose a reason for hiding this comment

bader Jan 15, 2020

Choose a reason for hiding this comment

Fznamznon Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

bader Jan 17, 2020

Choose a reason for hiding this comment

Fznamznon Jan 17, 2020

Choose a reason for hiding this comment

Ruyk commented Jan 15, 2020

bader commented Jan 15, 2020

rolandschulz commented Jan 15, 2020 • edited Loading

Ruyk commented Jan 16, 2020

Fznamznon commented Jan 17, 2020

bader left a comment

Choose a reason for hiding this comment

rolandschulz commented Jan 17, 2020

rolandschulz Jan 18, 2020

Choose a reason for hiding this comment

Fznamznon Jan 20, 2020

Choose a reason for hiding this comment

Ruyk Jan 21, 2020

Choose a reason for hiding this comment

bader Jan 21, 2020

Choose a reason for hiding this comment

Fznamznon commented Jan 21, 2020

bader left a comment

Choose a reason for hiding this comment

Ruyk commented Jan 21, 2020

Fznamznon commented Jan 22, 2020

Fznamznon commented Jan 15, 2020 •

edited by bader

Loading

Fznamznon Jan 17, 2020 •

edited

Loading

rolandschulz commented Jan 15, 2020 •

edited

Loading