Page MenuHomePhabricator

[LTO][Legacy] Add API to set result type to assembly
Needs ReviewPublic

Authored by w2yehia on Fri, Sep 25, 8:14 AM.

Details

Summary

I'm trying to make libLTO (https://llvm.org/docs/LinkTimeOptimization.html#liblto) generate an assembly file instead of object format.
Right now, the --filetype=asm option has no effect in libLTO. This option is used by tools such as llvm-lto and llc.
libLTO directly interacts with LTOCodeGenerator, which does not use the option but has a method to override the default file type (CGFT_ObjectFile) .
My current solution is to add a new function to lto.h that sets the filetype to asm.

Another solution is to have one of the libLTO functions that create LTOCodeGenerator, and after the ParseCommandLineOptions has run, to query the --filetype option and call LTOCodeGenerator::setFileType if the option was specified (I haven't tried that, but shud work in theory).

Diff Detail

Event Timeline

w2yehia created this revision.Fri, Sep 25, 8:14 AM
w2yehia requested review of this revision.Fri, Sep 25, 8:14 AM

Can you clarify your motivation for this? libLTO is used as an interface for linker and linkers, in general, do not understand assembly files.

Can you clarify your motivation for this? libLTO is used as an interface for linker and linkers, in general, do not understand assembly files.

Hi Steven, the object code codegen for the platform I'm using is not 100% functional, but the assembly generation is in much better shape, so we're taking the longer trip of calling the assembler to get the final .o file.

Note libLTO API needs to be stable so:

  • If you need to add API, you need to bump API version, etc. Please refer to other commits that add APIs to see what needs to be done.
  • If this is just a temporary workaround, I would resist make this change to bake in this API that will not be used in the future.

I'm not the right person to review this change, as we don't use the old LTO API at all. But I'll just add a quick note that there is a way to pass this through the new LTO API (lto::Config::CGFileType), and options to set it from lld/gold-plugin/llvm-lto2.

  • If this is just a temporary workaround, I would resist make this change to bake in this API that will not be used in the future.

This seems to be conflating the immediate reason with the more general value of having such an interface. The interface that @tejohnson's earlier comment mentions presumably exists because there is value in having such an interface.

  • If this is just a temporary workaround, I would resist make this change to bake in this API that will not be used in the future.

This seems to be conflating the immediate reason with the more general value of having such an interface. The interface that @tejohnson's earlier comment mentions presumably exists because there is value in having such an interface.

C++ interface is different from a libLTO C API which has additional requirement for compatibility. You can use the new C++ interface mentioned by @tejohnson or use the underlying c++ API for legacy API CG->setFileType() if you want.
I am fine with adding such a C API but I want to make sure you think through all the options, not just adding a new stable C API that is going to be thrown away.

I am fine with adding such a C API but I want to make sure you think through all the options, not just adding a new stable C API that is going to be thrown away.

We are using the C API because we need the stability. But yes, this points out that a linker using this would always be going through the assembly path even if the backend evolves past the point where that is a good idea.

In that case, please increment LTO_API_VERSION. If you think the API needs more documentation, please add to docs/LinkTimeOptimization.rst

@steven_wu what about the alternative solution where we make libLTO query the --filetype command line option. This will not require adding a new function to the API, but it does change the behavior of existing functions (lto_codegen_write_merged_modules, lto_codegen_compile, lto_codegen_optimize, lto_codegen_compile_optimized, lto_codegen_compile_to_file)

diff --git a/llvm/tools/lto/lto.cpp b/llvm/tools/lto/lto.cpp
index b680714..8251f7b 100644
--- a/llvm/tools/lto/lto.cpp
+++ b/llvm/tools/lto/lto.cpp
@@ -157,20 +157,23 @@
 
 // Convert the subtarget features into a string to pass to LTOCodeGenerator.
 static void lto_add_attrs(lto_code_gen_t cg) {
   LTOCodeGenerator *CG = unwrap(cg);
   auto MAttrs = codegen::getMAttrs();
   if (!MAttrs.empty()) {
     std::string attrs = join(MAttrs, ",");
     CG->setAttr(attrs);
   }
 
+  if (auto FT = codegen::getExplicitFileType())
+    CG->setFileType(FT.getValue());
+
   if (OptLevel < '0' || OptLevel > '3')
     report_fatal_error("Optimization level must be between 0 and 3");
   CG->setOptLevel(OptLevel - '0');
   CG->setFreestanding(EnableFreestanding);
 }

@steven_wu what about the alternative solution where we make libLTO query the --filetype command line option. This will not require adding a new function to the API, but it does change the behavior of existing functions (lto_codegen_write_merged_modules, lto_codegen_compile, lto_codegen_optimize, lto_codegen_compile_optimized, lto_codegen_compile_to_file)

diff --git a/llvm/tools/lto/lto.cpp b/llvm/tools/lto/lto.cpp
index b680714..8251f7b 100644
--- a/llvm/tools/lto/lto.cpp
+++ b/llvm/tools/lto/lto.cpp
@@ -157,20 +157,23 @@
 
 // Convert the subtarget features into a string to pass to LTOCodeGenerator.
 static void lto_add_attrs(lto_code_gen_t cg) {
   LTOCodeGenerator *CG = unwrap(cg);
   auto MAttrs = codegen::getMAttrs();
   if (!MAttrs.empty()) {
     std::string attrs = join(MAttrs, ",");
     CG->setAttr(attrs);
   }
 
+  if (auto FT = codegen::getExplicitFileType())
+    CG->setFileType(FT.getValue());
+
   if (OptLevel < '0' || OptLevel > '3')
     report_fatal_error("Optimization level must be between 0 and 3");
   CG->setOptLevel(OptLevel - '0');
   CG->setFreestanding(EnableFreestanding);
 }

That is not a bad idea but I think this is riskier than just adding a new API. Also, passing LTO option through -mllvm option (aka, through commanline option flags) is not officially supported.

I would go with a new C API.

Thanks Steven and Teresa.

Also, passing LTO option through -mllvm option (aka, through commanline option flags) is not officially supported.

Not sure what you mean by " through -mllvm option".
By the way, if you're using the GOLD plugin interface to invoke LTO, you just pass options thru -plugin-opt, no -mllvm. And in libLTO, you pass options through lto_codegen_debug_options C function.
Also, libLTO does query options defined in the LTO files (the function lto_add_attrs queries the -mattr option).

I'll take the new API approach then, unless there are objections from anyone else (shud we add more reviewers?)