yaxunl marked an inline comment as done. yaxunl added a comment. 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. ================ 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">; ---------------- jansvoboda11 wrote: > yaxunl wrote: > > tra wrote: > > > Should it be `BoolFOption` ? > > Yes. will fix > The `BoolFOption` marshalling multiclass should be only used for flags where > either the positive or the negative (or both) are -cc1 options and map to a > field in `CompilerInvocation`. > > Since this patch only deals with the driver (not the cc1 frontend) using > `BoolFOption` is not correct. Please, revert this change to the previous > state. > > I might need to explicitly call this out in the documentation > https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option. will do CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99683/new/ https://reviews.llvm.org/D99683 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
