[PATCH] D99683: [HIP] Support ThinLTO

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl added a comment.

In D99683#2664674 <https://reviews.llvm.org/D99683#2664674>, @tejohnson wrote:

> I haven't looked extensively yet, but why import noinline functions? Also, please add a patch description.

AMDGPU backend does not support linking of object files containing external symbols, i.e. one object file calling a function defined in another object file. Therefore the LLVM module passed to AMDGPU backend needs to contain definitions of all callees, even if a callee has noinline attribute. To support backends like this, the function importer needs to be able to import functions with noinline attribute. Therefore we add an LLVM option for allowing that, which is off by default. We have comments at line 70 of HIP.cpp about this.

Will add a patch description.



================
Comment at: clang/include/clang/Driver/Options.td:1904-1907
+def foffload_lto : Flag<["-"], "foffload-lto">, Flags<[CoreOption]>, Group<f_Group>,
+  HelpText<"Enable LTO in 'full' mode for offload compilation">;
+def fno_offload_lto : Flag<["-"], "fno-offload-lto">, Flags<[CoreOption]>, Group<f_Group>,
+  HelpText<"Disable LTO mode (default) for offload compilation">;
----------------
tra wrote:
> Should it be `BoolFOption` ? 
Yes. will fix


================
Comment at: clang/lib/Driver/Driver.cpp:623
 
+  llvm::errs() << LTOName << '\n';
   LTOMode = llvm::StringSwitch<LTOKind>(LTOName)
----------------
tra wrote:
> Leftover debug printout?
will remove


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4420
 
-    // Device-side jobs do not support LTO.
-    bool isDeviceOffloadAction = !(JA.isDeviceOffloading(Action::OFK_None) ||
-                                   JA.isDeviceOffloading(Action::OFK_Host));
-
-    if (D.isUsingLTO() && !isDeviceOffloadAction) {
+    // Device-side jobs do not support LTO, except AMDGPU
+    if (IsUsingLTO && (!IsDeviceOffloadAction || Triple.isAMDGPU())) {
----------------
tra wrote:
> Nit: rephrase it as `Only AMDGPU supports device-side LTO` ?
will do


================
Comment at: llvm/test/Transforms/FunctionImport/noinline.ll:4
+; RUN: opt -module-summary %p/Inputs/noinline.ll -o %t2.bc
+; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
+
----------------
tra wrote:
> I'd add a meaningful suffix to the binaries we'll use to run the checks on. E.g `%t3` -> `%t.lto.bc`, `%t2` -> `%t.inputs.noinline.bc`, `%t` -> `%t.main.bc`.
will rename %t and %t2 as suggested. However, llvm-lto will postfix the output file name with .thinlto.bc, therefore I would rename %t3 -> %t.summary


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

https://reviews.llvm.org/D99683