[PATCH] D112492: [HIP] Do not use kernel handle for MSVC target

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D112492#3101090 <https://reviews.llvm.org/D112492#3101090>, @tra wrote:

> As phrased, the summary would likely be rather confusing for anyone other than you and me.
>
>> Currently Visual Studio 2019 has a linker issue which causes linking error
>> when a template kernel is instantiated in different compilation units.
>
> It's not clear what exactly is the issue and what causes it.

Currently, the identical instantiation of a template kernel in different TU causes linking error about duplicate symbols on Windows.

In the beginning, I thought it was due to a bug in MSVC linker. However, further investigation shows that it is not a MSVC linker bug, but a bug of clang.

Basically, it is not sufficient to set linkonce_odr linkage to let MSVC linker merge symbols. The symbol needs to be in comdat sections. This is not required on Linux. However, it would be more consistent to always put kernel stubs and kernel handles with linkonce_odr linkage into comdat if the target supports it.

I will update this patch to fix the comdat for kernel stub and kernel handle.

>> On the other hand, it is unnecessary to prefix kernel stub for MSVC
>> target since the host and device compilation uses different mangling
>> ABI.
>
> This could use more details on why different mangling matters here. IIRC, on Linux where both host and device use the same mangling and HIP needed a way to tell apart the GPU-side kernels and their host-side stub. Different mangling makes it a non-issue.
>
>> This patch let clang not emit kernel handle for MSVC target to work around the linker issue.
>
> Again, without the back-story the jump from linking error to mangling differences to "let's not emit a handle" does not make much sense.
>
> I'd restructure it along the line of:
>
> - we emit host-side handles to match GPU-side kernels
> - the handles cause linking issues on windows because of X/Y/Z.
> - handles are not necessary on Windows, because of the different host/device mangling
> - not generating the handles avoids the linking issue on Windows.
>
> This prompts the question -- should/could handle generation be improved instead? Having identical behavior on all platforms would arguably be better than a platform-specific workaround.

With the fix of comdat issue, we should be able to use a consistent kernel launching mechanism for Linux and Windows. Since the debugger requests kernel stub to have a different name than the kernel, we have to let the kernel stub and kernel handle have different names. That mechanism will stay unchanged.

I also found that MSVC name mangling currently does not add device_stub prefix to the mangled name of kernel stubs. I will update this patch to fix that.

With these changes, we should have consistent name mangling for kernel stubs and kernel launching mechanism on Linux and Windows.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112492/new/

https://reviews.llvm.org/D112492