The linker options (e.g. pragma detect_mismatch) are intended for host compilation only, therefore disable it for device compilation.
Could you elaborate on why you want to disable this metadata? I think the original idea of llvm.linker.options was that it should be ignored if the back-end does not support it.
|441 ↗||(On Diff #185589)|
If we do need to disable it, it may be better to do it in
Fails how? AFAICT, for llvm.linker.options emitModuleMetadata() just creates an ELF section ".linker-options" which should just work.
Are you saying that some of HIP tools can't deal with the unknown section type?
Could you, please, give us a bit more context and provide more info for the questions @rjmccall and I asked before?
Is the problem specifically because autolink is not supported on device side? Or is disabling autolink just a convoluted way to avoid calling EmitModuleLinkOptions()?
If it's the former, then we should just disable it unconditionally -- we already filter out some other flags (e.g. asan).
If it's the latter, then tweaking autolink option behavior is just covering the problem. Sooner or later EmitModuleLinkOptions() will be called by something else and we'll be back to where we are now.
we need to disable autolink for device compilation because the linker options are intended for host compilation.
Previously I said the backend does not support linker option, which was incorrect. The diagnostic about invalid linker option was due to a clang codegen bug due to detect_mismatch, which I have a fix
Even with that fix, we still need to disable auto link for device compilation, since it is intended for host compilation.
Shouldn't it be true, considering that we do want to disable autolinking by default for device-side CUDA/HIP?
If we don't support autolinking at all for CUDA/HIP, perhaps we should just return true here.
This variable Default is to be used as the default value of OPT_fautolink. For device compilation we want the default value to be false. However if users want to force it to be true, we may still want to respect it.
You are correct. I've missed the ! in the return below. Don't never use double negation. :-/
Nit: Perhaps we can rename Default -> AutolinkEnabledByDefault because right now it's easy to misinterpret it as the default return value of the function. Maybe, even change the function itself to ShouldEnableAutolink().
LGTM in general. Nit about test name and test scope.
|1–5 ↗||(On Diff #227636)|
This appears to be specific to HIP on windows. If that's intended, then the file should be renamed to something like hip-ms-linker-options.cu. If the changes in functionality apply on other platforms, or with CUDA, the file name can remain as is, but it would be great to add some test runs for the other uses cases.
|1–5 ↗||(On Diff #227636)|
the change in functionality applies to HIP and CUDA on windows only. will rename it to ms-linker-options.cu and add test for CUDA when committing.