[PATCH] D99683: [HIP] Support ThinLTO

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

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

> In D99683#2669136 <https://reviews.llvm.org/D99683#2669136>, @yaxunl wrote:
>
>> In D99683#2669080 <https://reviews.llvm.org/D99683#2669080>, @tejohnson wrote:
>>
>>> In D99683#2669047 <https://reviews.llvm.org/D99683#2669047>, @yaxunl wrote:
>>>
>>>> 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.
>>>
>>> How does a non-LTO build work, or is (full) LTO currently required? Because with ThinLTO we only import functions that are externally defined but referenced in the current module. Also, when ThinLTO imports functions it makes them available_externally, which means they are dropped and made external symbols again after inlining. So anything imported but not inlined will go back to being an external symbol.
>>
>> AMDGPU backend by default uses full LTO for linking. It does not support non-LTO linking. Currently, it inlines all functions except kernels. However we want to be able to be able not to inline all functions. Is it OK to add an LLVM option to mark imported functions as linkonce_odr so that AMDGPU backend can keep the definitions of the imported functions?
>
> Actually AMDGPU backend will internalize all non-kernel functions before codegen. Those functions with available_externally linkage will have internal linkage before codegen, therefore they will not be dropped.

This raises some higher level questions for me:

First, how will you deal with other corner cases that won't or cannot be imported right now? While enabling importing of noinline functions and cranking up the threshold will get the majority of functions imported, there are cases that we still won't import (functions/vars that are interposable, certain funcs/vars that cannot be renamed, most non-const variables with non-trivial initializers).

Second, force importing of everything transitively referenced defeats the purpose of ThinLTO and would probably make it worse than regular LTO. The main entry module will need to import everything transitively referenced from there, so everything not dead in the binary, which should make that module post importing equivalent to a regular LTO module. In addition, every other module needs to transitively import everything referenced from those modules, making them very large depending on how many leaf vs non-leaf functions and variables they contain. What is the goal of doing ThinLTO in this case?


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

https://reviews.llvm.org/D99683