SPIR-V built-ins declarations rejected by the SYCL device compiler

[bader]

@Naghasan, I have problem with building libdevice library with my version of the SYCL compiler. "My version" includes improved "deferred diagnostics" patch from https://reviews.llvm.org/D70172, which seems to enable diagnostics #1026 for the TableGen-emitted declarations. There seems to be a bug in existing implementation.

Am I right that SPIR-V built-ins are emitted w/o sycl_device attribute? If so, the SYCL device compiler is supposed to emit an error for declaration w/o definition and not explicitly annotated with SYCL_EXTERNAL macro (aka sycl_device attribute). NOTE: original header replaced by TableGen file included required annotations.

Can we add sycl_device attribute to SPIR-V built-ins if -fsycl-is-device language option is set?
Any other ideas how to fix libdevice build?
Let me know if you want me to share the compiler with fixed deferred diagnostics.

Tagging @s-kanaev, @erichkeane, @Fznamznon.

[Naghasan]

Am I right that SPIR-V built-ins are emitted w/o sycl_device attribute?

No they are emitted with this attribute (set here), that would trigger SYCL diagnostics otherwise.

Can we add sycl_device attribute to SPIR-V built-ins if -fsycl-is-device language option is set?

This should be check regardless I guess, I'll make a PR.

[bader]

I think I found the problem. The attribute set by clang is "implicit", where as original code was explicit.
Diagnostics is emitted because implicit attribute is added by clang during "device code traversal" and compiler expects user to add this attribute rather than compiler will find that this built-in is required on the device.

There are two ways to fix this problem:

1. Emit explicit attribute for SPIR-V built-ins.
2. @Fznamznon, I think you recently change the way clang emits device code and I think we don't add implicit attribute any more, right? If so, we can consider updating the diagnostics and remove "explicit attribute" requirement from here.

[vladimirlaz]

@bader , @Fznamznon I faced the same issue during LLVM pulldown to intel/llvm and tried option 2 mentioned in Alexey's comment (061a2c6). It helps!!

[Fznamznon]

@Fznamznon, I think you recently change the way clang emits device code and I think we don't add implicit attribute any more, right? If so, we can consider updating the diagnostics and remove "explicit attribute" requirement from here.

Right, implicit sycl_device attribute is not needed for all the functions to emit them in device code anymore. There are two places where I left adding it implicitly:

• Applying indirectly_callable to a function

  llvm/clang/lib/Sema/SemaDeclAttr.cpp

  Line 4463 in d15de0b

  D->addAttr(SYCLDeviceAttr::CreateImplicit(S.Context));

  . I think we can safely remove it if we extend this condition

  llvm/clang/lib/AST/ASTContext.cpp

  Line 10194 in d15de0b

  if (LangOpts.SYCLIsDevice && !D->hasAttr<OpenCLKernelAttr>() &&

  by adding check for indirectly_callable attribute here (because indirectly_callable is supposed to force emitting of function like SYCL_EXTERNAL).
• Adding this attribute to generated OpenCL function

  llvm/clang/lib/Sema/SemaSYCL.cpp

  Line 1223 in d15de0b

  OpenCLKernel->addAttr(SYCLDeviceAttr::CreateImplicit(Context));

  . I think I just missed it and it can be safely removed.

I can prepare a patch if you want.

[bader]

@Fznamznon, I don't think we should change the code for indirectly callable or SYCL kernel functions.
Your analysis shows that we should definitely lift the restriction on explicit attribute.
If I understand it correctly, forward declaration annotated with [[indirectly_callable]] might still cause the error.

It seems to me that the right thing to do is to remove this line: https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaSYCL.cpp#L1455 and assume that even attribute is added implicitly to forward declaration, it's not a bug and compiler guarantees that function is available on the device.

Does it make sense to you?

[Fznamznon]

@Fznamznon, I don't think we should change the code for indirectly callable or SYCL kernel functions.
Your analysis shows that we should definitely lift the restriction on explicit attribute.
If I understand it correctly, forward declaration annotated with [[indirectly_callable]] might still cause the error.

It seems to me that the right thing to do is to remove this line: https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaSYCL.cpp#L1455 and assume that even attribute is added implicitly to forward declaration, it's not a bug and compiler guarantees that function is available on the device.

Does it make sense to you?

Okay. But I don't see the point in adding sycl_device attribute to generated OpenCL kernel. This is not needed for device code outlining anymore and it shouldn't affect any diagnostics.

[bader]

But I don't see the point in adding sycl_device attribute to generated OpenCL kernel. This is not needed for device code outlining anymore and it shouldn't affect any diagnostics.

Right.

[bader]

@vladimirlaz, are you going to commit 061a2c6 in scope of #1432?

[vladimirlaz]

@bader, yes until corresponding check is not submitted earlier