[PATCH] D99683: [HIP] Support ThinLTO

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added a subscriber: tejohnson.
tra added a comment.
This revision is now accepted and ready to land.

LGTM in general. Please give LTO folks some time to chime in case they have any feedback.

@tejohnson: Just a FYI that we're tinkering with LTO on GPUs here.



================
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">;
----------------
Should it be `BoolFOption` ? 


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


================
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())) {
----------------
Nit: rephrase it as `Only AMDGPU supports device-side LTO` ?


================
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
+
----------------
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`.


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

https://reviews.llvm.org/D99683