Page MenuHomePhabricator

[CUDA][HIP] Disable emitting llvm.linker.options in device compilation
ClosedPublic

Authored by yaxunl on Feb 6 2019, 10:53 AM.

Details

Summary

The linker options (e.g. pragma detect_mismatch) are intended for host compilation only, therefore disable it for device compilation.

Diff Detail

Event Timeline

yaxunl created this revision.Feb 6 2019, 10:53 AM
tra added a comment.Feb 6 2019, 11:11 AM

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.

lib/CodeGen/CodeGenModule.cpp
441 ↗(On Diff #185589)

If we do need to disable it, it may be better to do it in
ShouldDisableAutolink() function in clang/lib/Driver/ToolChains/Clang.cpp

In D57829#1387412, @tra wrote:

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.

If backend does not support it, it goes to TargetLoweringObjectFileELF::emitModuleMetadata and causes codegen to fail.

tra added a comment.Feb 6 2019, 11:27 AM
In D57829#1387412, @tra wrote:

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.

If backend does not support it, it goes to TargetLoweringObjectFileELF::emitModuleMetadata and causes codegen to fail.

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?

Also, why is this HIP-specific? I thought the toolchain was largely shared with CUDA and there were just a few runtime differences.

yaxunl abandoned this revision.Sep 26 2019, 10:10 AM
yaxunl marked an inline comment as done.

no longer needed

yaxunl reclaimed this revision.Wed, Oct 30, 8:19 PM

we still need this

yaxunl updated this revision to Diff 227216.Wed, Oct 30, 9:02 PM
yaxunl retitled this revision from [HIP] Disable emitting llvm.linker.options in device compilation to [CUDA][HIP] Disable emitting llvm.linker.options in device compilation.
yaxunl edited the summary of this revision. (Show Details)

use -fno-autolink to disable linker option for device compilation.

tra added a comment.Thu, Oct 31, 11:32 AM

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.

In D57829#1729094, @tra wrote:

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

https://reviews.llvm.org/D69678

Even with that fix, we still need to disable auto link for device compilation, since it is intended for host compilation.

tra added inline comments.Thu, Oct 31, 2:08 PM
clang/lib/Driver/ToolChains/Clang.cpp
389–392

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.

yaxunl marked an inline comment as done.Thu, Oct 31, 6:06 PM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
389–392

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.

tra added inline comments.Fri, Nov 1, 10:15 AM
clang/lib/Driver/ToolChains/Clang.cpp
389–392

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().

yaxunl marked an inline comment as done.Fri, Nov 1, 4:59 PM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
389–392

will change it to ShouldEnableAutolink

yaxunl updated this revision to Diff 227636.Sun, Nov 3, 2:17 PM

revised by Artem's comments. Also fix test since linker option only supported on windows.

tra accepted this revision.Mon, Nov 4, 10:46 AM

LGTM in general. Nit about test name and test scope.

clang/test/CodeGenCUDA/linker-options.cu
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.

This revision is now accepted and ready to land.Mon, Nov 4, 10:46 AM
yaxunl marked an inline comment as done.Mon, Nov 4, 11:10 AM
yaxunl added inline comments.
clang/test/CodeGenCUDA/linker-options.cu
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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 4, 8:28 PM