[SYCL][NATIVECPU] Fix assert in MS name mangler #11103
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // This test ensures the native-cpu device generates the expected kernel names, | ||
| // and that the MS mangler doesn't assert on the code below. | ||
|
|
||
| // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -aux-triple x86_64-pc-windows-msvc -I %S/Inputs -fsycl-is-device -fsycl-is-native-cpu -emit-llvm -o - -x c++ %s | FileCheck %s | ||
| // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -aux-triple x86_64-unknown-linux-gnu -I %S/Inputs -fsycl-is-device -fsycl-is-native-cpu -emit-llvm -o - -x c++ %s | FileCheck %s | ||
| // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -aux-triple x86_64-pc-windows-msvc -I %S/Inputs -fsycl-is-device -fsycl-is-native-cpu -emit-llvm -o - -x c++ %s | FileCheck %s | ||
| // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -aux-triple x86_64-unknown-linux-gnu -I %S/Inputs -fsycl-is-device -fsycl-is-native-cpu -emit-llvm -o - -x c++ %s | FileCheck %s | ||
| // Todo: check other cpus | ||
|
|
||
| #include "sycl.hpp" | ||
|
|
||
| struct name1; | ||
|
|
||
| void test(sycl::handler &h) { | ||
| h.parallel_for_work_group<name1>(sycl::range<1>(2),sycl::range<1>(1), [=](sycl::group<1> G) {}); | ||
| } | ||
|
|
||
| // CHECK: void @_ZTS5name1( | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. All SYCL targets need to generate the same name for a kernel to enable lookup in different device binaries. All SYCL targets in DPC++ use the Itanium mangling for kernel names. We still need the MS mangler fix though because when targeting There was a problem hiding this comment. I'm not sure I'm following. We generate a mangled name for the MS C++ ABI that is never used? If so, wouldn't a better fix be to skip generating such a mangled name in the first place? There was a problem hiding this comment.
We were considering this but it would probably be more intrusive to query/skip/change the kernel name mangling only for NativeCPU kernels targeting Windows than just handling the Also, There was a problem hiding this comment. Just to close out this thread... I've still been struggling to follow these explanations, but as mentioned elsewhere, I won't hold this review up since the change should be harmless, but we'll need to revisit this as part of SYCL upstreaming; the use of |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok to me but @premanandrao can you also please take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a lot to learn in this area. I don't understand why OpenCL is relevant for the added test case. Is OpenCL always used for
-fsycl-is-native-cpu?Mangling also seems to be missing for IA64 ABIs. See
CXXNameMangler::getCallingConvQualifierName()inclang/lib/AST/ItaniumMangle.cpp. Why isn't a similar change needed there? The test exercises Linux as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has anyone tried to determine what, if any, mangling is used by MSVC for OpenCL kernels?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was that this is the calling convention for kernel. I think that when using
-fsycl-is-native-cpudevice compilation is done with the host triple and this results in us entering MS mangler for the kernel. @uwedolinsky did I get that correct?This is a good point. Why is this not an issue for Linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCL is not relevant for this test, but since the frontend attaches the clang
CC_OpenCLKernelconvention (independent from C++ ABI and target) to the kernel declaration here, we need to handle that convention in the currently used name mangler (which is the MS mangler when targeting Windows).-fsycl-is-native-cpudoes not use or enable OpenCL in compiler or runtime.NativeCPUhas no dependency on OpenCL.CXXNameMangler::getCallingConvQualifierName()already handles theCC_OpenCLKerneland other conventions here (without providing mangling), but this doesn't even seem to be called on SYCL kernels when targeting the IA64 ABI, so that problem will not occur on that ABI.We wanted the test to also cover other ABIs and different combinations of cpu triples (for possible cross compilation) to ensure they don't have similar problems. Some of these invocations may not exercise the fix but are still worth testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining. I think I'm convinced this change is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still isn't clear to me that this is the right change. Why is the
CC_OpenCLKernelattached to SYCL kernels in the first place? Can we just stop attaching them to SYCL kernels? I think we should understand why they are needed before choosing to ignore them.MSVC doesn't directly support the OpenCL language. From what I can tell, OpenCL kernels are always resolved by a call to
clCreateKernel()that passes the name of the kernel as a string. Overloaded kernels are supported (at least by some implementations) by allowing a custom mangled name to be specified via an attribute. It seems any kind of mangling is therefore implementation dependent and thus, the Microsoft ABI doesn't specify one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably because of historical reasons since SYCL initially targeted OpenCL, many data structures and identifiers may have been inspired by it and SYCL kernels were implemented as OpenCL kernels. Also, querying the calling convention is one way to identify kernels.
Probably, but that would likely lead to a much larger, potentially intrusive SYCL frontend change I mentioned previously and I was trying to avoid that. If NativeCPU is the only SYCL target that has this issue, and there are no other issues elsewhere with using this convention "under the hood", it may not be worth making that large a change and instead it may be easier to just handle the assert.
Please note that this PR does not specify any MS mangling, it merely prevents the assert in the MS mangler triggered for the
CC_OpenCLKernelconvention.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for being responsive to my comments. I won't hold this review up further; the change seems unlikely to cause any problems in the short term. However, we will have to revisit all of this as part of the SYCL upstreaming effort as the reuse of
CC_OpenCLKernelfor SYCL here is quite confusing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if the clang infrastructure for OpenCL isn't used to implement SYCL kernels (not even when targeting OpenCL), then this "reuse" of
CC_OpenCLKernelcould probably be avoided. It may be possible to instead use the information fromSYCLKernelAttror other SYCL-related clang attributes that are currently attached to SYCL kernels in the AST. But such potentially larger refactoring is probably better done as a separate PR.