[SYCL] Move function pointer diagnostics to BuildResolvedCallExpr

[Naghasan]

The patch moves the function pointer related diagnostics from DiagDeviceFunction::VisitCallExpr class to BuildResolvedCallExpr and use a delayed diagnostic.

This allow the compiler to provide the template instantiation trace when emitting the diagnostic.

[premanandrao]

What diagnostics are possible with this change that weren't possible before? Please add test case showing them.

[Naghasan]

It just didn't display the instantiation stack, making things difficult to understand where it was coming from. Updated test to check the note emission.

[Fznamznon]

That is a nice improvement! Could you please also check that the error is not emitted from the host code? Delayed diagnostics can be buggy.

[Naghasan]

Could you please also check that the error is not emitted from the host code?

Sure, I added a no-diagnostics check in constexpr-function-pointer.cpp

[Fznamznon]

Thanks!

[Naghasan]

@premanandrao do you have comments on the PR ?

[Naghasan]

ping @premanandrao

[Naghasan]

@intel/llvm-gatekeepers I believe this ready (comments addressed and PR approved)

[KornevNikita]

@Naghasan this commit broke the sycl-cts/test_language test (see):

/__w/llvm/llvm/khronos_sycl_cts/tests/language/constant_evaluation.cpp:52:22: error: SYCL kernel cannot call through a function pointer
   52 |         auto other = p(5) + f(6);
      |                      ^

Is it a test issue?

[igchor]

@Naghasan This is also causing issues for llama.cpp. They pass function pointers as a template parameter and this triggers an error after this patch. See here: https://github.com/ggml-org/llama.cpp/blob/master/ggml/src/ggml-sycl/mmq.cpp#L1196

[aelovikov-intel]

Is it a test issue?

No, that's a bug in our implementation. KhronosGroup/SYCL-Docs#388 explicitly allowed that code.

[Naghasan]

Is it a test issue?

well it is, clang lit works but there is clearly holes in them

[aelovikov-intel]

Is it a test issue?

well it is, clang lit works but there is clearly holes in them

Can you clarify what you mean? CTS failure is real and not a test issue.

[pbalcer]

Is it a test issue?

well it is, clang lit works but there is clearly holes in them

Can you clarify what you mean? CTS failure is real and not a test issue.

Also, like @igchor said llama.cpp, which we use for performance testing, no longer compiles after this PR.

[Naghasan]

Can you clarify what you mean? CTS failure is real and not a test issue.

there is coverage in clang's tests :)

working on a fix BTW