[PATCH] D99683: [HIP] Support ThinLTO

Mon Apr 12 08:45:56 PDT 2021

tejohnson added a comment.

In D99683#2677357 <https://reviews.llvm.org/D99683#2677357>, @yaxunl wrote:

> Any other concerns? Thanks.

I have some concerns around the fragility of this, for the reasons I mentioned earlier where it may not always be able to import everything needed.

> We will document the limitation of thinLTO support of HIP toolchain and recommend users not to use thinLTO in those corner cases.

Where will this be documented?

My concern is that we start getting bugs filed for these corner cases, and it burns a bunch of someone's time to dig in only to discover that it is an unsupported corner case. Since we can detect in the function importer when we cannot import something successfully, I think it would therefore be worthwhile to issue a hard error with a meaningful error message in the AMDGPU case.

================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4421
+    // Only AMDGPU supports device-side LTO
+    if (IsUsingLTO && (!IsDeviceOffloadAction || Triple.isAMDGPU())) {
       Args.AddLastArg(CmdArgs, options::OPT_flto, options::OPT_flto_EQ);
----------------
Should there be an error (or is there one already) emitted somewhere if LTO is requested along with device offloading and this isn't AMDGPU?

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

https://reviews.llvm.org/D99683