[lld] Support --lto-emit-asm and --plugin-opt=emit-asm

[hoyFB]

Summary

The switch --plugin-opt=emit-asm can be used with the gold linker to dump the final assembly code generated by LTO in a user-friendly way. Unfortunately it doesn't work with lld. I'm hooking it up with lld. With that switch, lld emits assembly code into the output file (specified by -o) and if there are multiple input files, each of their assembly code will be emitted into a separate file named by suffixing the output file name with a unique number, respectively. The linking then stops after generating those assembly files.

[hoyFB]

Updating D77231: [lld] Support -emit-asm with LTO

[MaskRay]

Test Plan: ninja check-lld

This can be omitted as it does add value to the description. Almost every change will need check-lld (or check-lld-elf during development if you are confident non-ELF is not affected.)

[MaskRay]

lld/test/ELF/lto/emit-asm.ll

4

Drop -m elf_x86_64 and use --lto-emit-asm

ld.lld --lto-emit-asm -shared %t.o -o - | FileCheck %s

9

Check the contents of %t2.s and %t2.s1, respectively.

[hoyFB]

lld/test/ELF/lto/emit-asm.ll

9

Now sure if the partition stably places f1 in the first assembly and f2 in the second. So I'm using CHECK-DAG on the merged output. What do you think?

[hoyFB]

Updating D77231: [lld] Support -emit-asm with LTO

[grimar]

lld/ELF/Driver.cpp

891

It looks inconsistent with another OPT_lto* options we have:

  config->ltoAAPipeline = args.getLastArgValue(OPT_lto_aa_pipeline);
  config->ltoCSProfileGenerate = args.hasArg(OPT_lto_cs_profile_generate);
  config->ltoCSProfileFile = args.getLastArgValue(OPT_lto_cs_profile_file);
...

Should it be called config->ltoEmitAsm to be consistent?

Or, instead, it could be args.hasArg(OPT_plugin_opt_emit_asm, false), what also would make sense it seems.

What do other reviewers think?

1963

That the last 2 comments have a large common part. Also it seems that if thinLTOIndexOnly, emitLLVM and emitAsm were added at once and not separatelly, we would not split them. I think it might be better to rewrite these 3 comments into a single one and do:

// <A shorter and nicer comment here>
if (config->thinLTOIndexOnly || config->emitLLVM || config->emitAsm)
  return;

[grimar]

lld/ELF/Driver.cpp

1963

"That the last 2..." -> "Last 2..."

[hoyFB]

lld/ELF/Driver.cpp

891

Naming it `config->ltoEmitAsm`` sounds good to me. If you look at the relationship between `OPT_lto*` and `OPT_plugin_opt_*`` in lld, the later is always an alias of the former.

1963

Sounds good.

[hoyFB]

lld/ELF/Driver.cpp

891

Well, on the second thought, I'm thinking maybe just removing the --lto-emit-asm switch and only having --plugin-opt=emit-asm, since it is not lld-exclusive. What do you think?

[MaskRay]

lld/ELF/Driver.cpp

891

--plugin-opt=* is for compatibility with gold. gold uses LLVMgold.so and the plugin interface does not allow insert arbitrary option on the fly. @pcc started to use --lto-* in D18667

Maybe he has some thoughts here.

1963

if (config->thinLTOIndexOnly || config->emitLLVM || config->emitAsm) return;

+1

lld/ELF/LTO.cpp

338

To be honest, the output file naming (--save-temps and the new emit-asm) here also looks strange to me. Why is 1 special cased? @pcc @tejohnson

[hoyFB]

lld/ELF/LTO.cpp

338

My understanding is that output 0 has no suffix on the output file name, and suffix starts from output 1. Is that your question?

[tejohnson]

lld/ELF/Driver.cpp

891

For compatibility, please do add support for the plugin-opt version, even if it is just an alias for a new --lto-emit-asm.

lld/ELF/LTO.cpp

338

I think @pcc added that handling to gold-plugin, but I believe it is so there is no suffix when we are doing regular LTO where there is one task.

[hoyFB]

Updating D77231: [lld] Support -emit-asm with LTO

[grimar]

lld/ELF/Driver.cpp

891

Well, on the second thought, I'm thinking maybe just removing the --lto-emit-asm switch and only having --plugin-opt=emit-asm, since it is not lld-exclusive. What do you think?

Seems nobody yet said something against this and it is anyways easy to add an additional alias option later if needed.
So looks like only having --plugin-opt=emit-asm for now should be fine.

@pcc started to use --lto-* in D18667. Maybe he has some thoughts here.

+1, I'd also would like to hear Peter's opinion.

[hoyFB]

Ping... I'm checkout out to see if you have more feedbacks for this change. Thanks!

[MaskRay]

Mostly good, with a few nits.

lld/ELF/Options.td

544

--lto-emit-asm

lld/test/ELF/lto/emit-asm.ll

2

; REQUIRES: x86

4

Nit: You can join the two lines

6

cat %t2.s %t2.s1 | FileCheck %s

7

It may be helpful to add a test demonstrating that --lto-emit-asm --save-temps can be use together.

[hoyFB]

Updating D77231: [lld] Support --lto-emit-asm and --plugin-opt=emit-asm

lld/test/ELF/lto/emit-asm.ll

7

Sounds good. I'm checking one of the .bc files are an output of --save-temps.

[MaskRay]

lld/ELF/Driver.cpp

920

Delete , false which is redundant

[hoyFB]

Updating D77231: [lld] Support --lto-emit-asm and --plugin-opt=emit-asm

[MaskRay]

lld/ELF/Options.td

544

This is not done.

[hoyFB]

lld/ELF/Options.td

544

Yeah, I overlooked this. I'm a bit confused here. I'm seeing every other lto switche has only one dash. What's the convention here?

[MaskRay]

lld/ELF/Options.td

544

I fixed Alias for in 42bb5cc502da0e01e5e714800dbec7d13603d399.

GNU ld has very loose parsing rules. It accepts either single-dash or double-dash options. For compatibility we have to do the same. For commonly used single-dash options e.g. -shared -pie (Their compiler driver counterparts use this form), I stick with them. For everything unclear, I use the double dash form.

-lto-... is bad because it conflicts with -l

[hoyFB]

Updating D77231: [lld] Support --lto-emit-asm and --plugin-opt=emit-asm

lld/ELF/Options.td

544

Gotcha, thanks for the explanation.

[hoyFB]

Please let me know any more feedbacks. I'm planning to land this shortly. Thanks!

[MaskRay]

In D77231#1987231, @hoyFB wrote:

Please let me know any more feedbacks.

Mostly good correct. @tejohnson ^^

I'm planning to land this shortly. Thanks!

This sentence does not look right. There are still a few comments not addressed. One is a critical one: ; REQUIRES: x86. Without it the test will fail on a built LLVM with the x86 target disabled. Some people maintaining ARM bots will assuredly revert your change.

[hoyFB]

lld/test/ELF/lto/emit-asm.ll

2

Oops, I overlooked this one. Why doesn't it work on non-X86?

[hoyFB]

Updating D77231: [lld] Support --lto-emit-asm and --plugin-opt=emit-asm

[hoyFB]

Updating D77231: [lld] Support --lto-emit-asm and --plugin-opt=emit-asm

[MaskRay]

lld/ELF/Options.td

486

F

J is a joined option, which expects an argument (which can be empty).

lld/test/ELF/lto/emit-asm.ll

5

Join the two lines.

I mentioned this in a previous comment.

[MaskRay]

lld/ELF/Options.td

543

Move it before emit_llvm for local lexicographical order.

[MaskRay]

@hoyFB LLVM is very loose by giving commit access to everyone, but we are expecting people to act in good faith. Your initial action (adding a colleague (with all due respect) who has no contribution to lld yet) and your word I'm planning to land this shortly made me very concerned that you wanted to bypass the usual review process. (I would not have overreacted if I had seen some lld contribution from you)

[hoyFB]

Updating D77231: [lld] Support --lto-emit-asm and --plugin-opt=emit-asm

[hoyFB]

In D77231#1995662, @MaskRay wrote:

@hoyFB LLVM is very loose by giving commit access to everyone, but we are expecting people to act in good faith. Your initial action (adding a colleague (with all due respect) who has no contribution to lld yet) and your word I'm planning to land this shortly made me very concerned that you wanted to bypass the usual review process. (I would not have overreacted if I had seen some lld contribution from you)

@MaskRay I think there is a misunderstanding going on here. I was hoping to have this change in sooner because we really need it on our side. That being said, we didn't intend to bypass any code reviewers to commit the change silently, as you can see that the change has been pending here for three weeks and all feedbacks from all reviewers have been addressed. If by an accident there are more feedbacks after this is committed, we will always address them in a new change or revert this change.

lld/test/ELF/lto/emit-asm.ll

5

Line 3 and Line 4?

[hoyFB]

Updating D77231: [lld] Support --lto-emit-asm and --plugin-opt=emit-asm

[MaskRay]

Thanks for persisting. The latest version looks good to me. Give some time for @grimar or @tejohnson to sign off since they've commented.

[tejohnson]

lgtm

[grimar]

LGTM

[hoyFB]

@MaskRay @grimar @tejohnson Thank you all for your great code reviews! I'm going to close this.

[MaskRay]

In D77231#2005752, @hoyFB wrote:

@MaskRay @grimar @tejohnson Thank you all for your great code reviews! I'm going to close this.

Delete unnecessary Phabricator tags with:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}

[hoyFB]

In D77231#2005866, @MaskRay wrote:

In D77231#2005752, @hoyFB wrote:

@MaskRay @grimar @tejohnson Thank you all for your great code reviews! I'm going to close this.

Delete unnecessary Phabricator tags with:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}

Thanks for the tips. seems a bit late. The change has just been committed. I'll do this for future changes. Do you still want amend for this commit?

[MaskRay]

Phabricator added some tags automatically. Among them, Summary: Reviewers: Tags: are considered not useful (discussed on a previous llvm-dev thread).
Reviewed-by: is useful and should be retained.

It is said that we can patch server-side Phabricator to hide these tags: http://lists.llvm.org/pipermail/llvm-dev/2020-April/140810.html
but no administrator has made action here.

I just train myself to use arcfilter; git fetch origin master && git rebase origin/master; ninja -C /path/to/build check-lld-elf && git push origin HEAD:master

lld/ELF/Config.h

struct Configuration {
   bool gnuHash = false;
   bool gnuUnique;
   bool hasDynamicList = false;
   bool hasDynSymTab;
   bool ignoreDataAddressEquality;
   bool ignoreFunctionAddressEquality;
   bool ltoCSProfileGenerate;
   bool ltoDebugPassManager;
+  bool ltoEmitAsm;
   bool ltoNewPassManager;
   bool ltoUniqueBBSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
   bool mmapOutputFile;
   bool nmagic;
   bool noDynamicLinker = false;

lld/ELF/Driver.cpp

static void readConfigs(opt::InputArgList &args) {
   config->dependentLibraries = args.hasFlag(OPT_dependent_libraries, OPT_no_dependent_libraries, true);
   config->disableVerify = args.hasArg(OPT_disable_verify);
   config->discard = getDiscard(args);
   config->dwoDir = args.getLastArgValue(OPT_plugin_opt_dwo_dir_eq);
   config->dynamicLinker = getDynamicLinker(args);
   config->ehFrameHdr =
       args.hasFlag(OPT_eh_frame_hdr, OPT_no_eh_frame_hdr, false);
   config->emitLLVM = args.hasArg(OPT_plugin_opt_emit_llvm, false);
   config->emitRelocs = args.hasArg(OPT_emit_relocs);
   config->callGraphProfileSort = args.hasFlag(
       OPT_call_graph_profile_sort, OPT_no_call_graph_profile_sort, true);
   config->enableNewDtags =
       args.hasFlag(OPT_enable_new_dtags, OPT_disable_new_dtags, true);
   config->entry = args.getLastArgValue(OPT_entry);
   config->executeOnly =
       args.hasFlag(OPT_execute_only, OPT_no_execute_only, false);
   config->exportDynamic =
config->ignoreDataAddressEquality =
       args.hasArg(OPT_ignore_data_address_equality);
   config->ignoreFunctionAddressEquality =
       args.hasArg(OPT_ignore_function_address_equality);
   config->init = args.getLastArgValue(OPT_init, "_init");
   config->ltoAAPipeline = args.getLastArgValue(OPT_lto_aa_pipeline);
   config->ltoCSProfileGenerate = args.hasArg(OPT_lto_cs_profile_generate);
   config->ltoCSProfileFile = args.getLastArgValue(OPT_lto_cs_profile_file);
   config->ltoDebugPassManager = args.hasArg(OPT_lto_debug_pass_manager);
+  config->ltoEmitAsm = args.hasArg(OPT_lto_emit_asm);
   config->ltoNewPassManager = args.hasArg(OPT_lto_new_pass_manager);
   config->ltoNewPmPasses = args.getLastArgValue(OPT_lto_newpm_passes);
   config->ltoWholeProgramVisibility =
       args.hasArg(OPT_lto_whole_program_visibility);
   config->ltoo = args::getInteger(args, OPT_lto_O, 2);
   config->ltoObjPath = args.getLastArgValue(OPT_lto_obj_path_eq);
   config->ltoPartitions = args::getInteger(args, OPT_lto_partitions, 1);
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
   // Symbol resolution finished. Report backward reference problems.
   reportBackrefs();
   if (errorCount())
     return;
 
   // If -thinlto-index-only is given, we should create only "index
   // files" and not object files. Index file creation is already done
   // in addCombinedLTOObject, so we are done if that's the case.
-  if (config->thinLTOIndexOnly)
-    return;
-
-  // Likewise, --plugin-opt=emit-llvm is an option to make LTO create
-  // an output file in bitcode and exit, so that you can just get a
-  // combined bitcode file.
-  if (config->emitLLVM)
+  // Likewise, --plugin-opt=emit-llvm and --plugin-opt=emit-asm are the
+  // options to create output files in bitcode or assembly code
+  // repsectively. No object files are generated.
+  if (config->thinLTOIndexOnly || config->emitLLVM || config->ltoEmitAsm)
     return;
 
   // Apply symbol renames for -wrap.
   if (!wrapped.empty())
     wrapSymbols(wrapped);
 
   // Now that we have a complete list of input files.
   // Beyond this point, no new files are added.
   // Aggregate all input sections into one place.
   for (InputFile *f : objectFiles)
     for (InputSectionBase *s : f->getSections())
       if (s && s != &InputSection::discarded)
         inputSections.push_back(s);
   for (BinaryFile *f : binaryFiles)
     for (InputSectionBase *s : f->getSections())

lld/ELF/LTO.cpp

static lto::Config createConfig() {
   if (config->emitLLVM) {
     c.PostInternalizeModuleHook = [](size_t task, const Module &m) {
       if (std::unique_ptr<raw_fd_ostream> os = openFile(config->outputFile))
         WriteBitcodeToFile(m, *os, false);
       return false;
     };
   }
 
+  if (config->ltoEmitAsm)
+    c.CGFileType = CGFT_AssemblyFile;
+
   if (config->saveTemps)
     checkError(c.addSaveTemps(config->outputFile.str() + ".",
                               /*UseInputModulePath*/ true));
   return c;
 }
 
 BitcodeCompiler::BitcodeCompiler() {
   // Initialize indexFile.
std::vector<InputFile *> BitcodeCompiler::compile() {
   }
 
   if (config->saveTemps) {
     saveBuffer(buf[0], config->outputFile + ".lto.o");
     for (unsigned i = 1; i != maxTasks; ++i)
       saveBuffer(buf[i], config->outputFile + Twine(i) + ".lto.o");
   }
 
+  if (config->ltoEmitAsm) {
+    saveBuffer(buf[0], config->outputFile);
+    for (unsigned i = 1; i != maxTasks; ++i)
+      saveBuffer(buf[i], config->outputFile + Twine(i));
+    return {};
+  }
+
   std::vector<InputFile *> ret;
   for (unsigned i = 0; i != maxTasks; ++i)
     if (!buf[i].empty())
       ret.push_back(createObjectFile(MemoryBufferRef(buf[i], "lto.tmp")));
 
   for (std::unique_ptr<MemoryBuffer> &file : files)
     if (file)
       ret.push_back(createObjectFile(*file));
   return ret;
 }
 
 } // namespace elf
 } // namespace lld

lld/ELF/Options.td

 def: JoinedOrSeparate<["-"], "u">, Alias<undefined>, HelpText<"Alias for --undefined">;
 def: Flag<["-"], "V">, Alias<version>, HelpText<"Alias for --version">;
 
 // LTO-related options.
 def lto_aa_pipeline: J<"lto-aa-pipeline=">,
   HelpText<"AA pipeline to run during LTO. Used in conjunction with -lto-newpm-passes">;
 def lto_debug_pass_manager: F<"lto-debug-pass-manager">,
   HelpText<"Debug new pass manager">;
+def lto_emit_asm: F<"lto-emit-asm">,
+  HelpText<"Emit assembly code">;
 def lto_new_pass_manager: F<"lto-new-pass-manager">,
   HelpText<"Use new pass manager">;
 def lto_newpm_passes: J<"lto-newpm-passes=">,
   HelpText<"Passes to run during LTO">;
 def lto_O: J<"lto-O">, MetaVarName<"<opt-level>">,
   HelpText<"Optimization level for LTO">;
 def lto_partitions: J<"lto-partitions=">,
   HelpText<"Number of LTO codegen partitions">;

 def thinlto_prefix_replace_eq: J<"thinlto-prefix-replace=">;
 
 def: J<"plugin-opt=O">, Alias<lto_O>, HelpText<"Alias for --lto-O">;
 def: F<"plugin-opt=debug-pass-manager">,
   Alias<lto_debug_pass_manager>, HelpText<"Alias for --lto-debug-pass-manager">;
 def: F<"plugin-opt=disable-verify">, Alias<disable_verify>, HelpText<"Alias for --disable-verify">;
 def plugin_opt_dwo_dir_eq: J<"plugin-opt=dwo_dir=">,
   HelpText<"Directory to store .dwo files when LTO and debug fission are used">;
+def plugin_opt_emit_asm: F<"plugin-opt=emit-asm">,
+  Alias<lto_emit_asm>, HelpText<"Alias for --lto-emit-asm">;
 def plugin_opt_emit_llvm: F<"plugin-opt=emit-llvm">;
 def: J<"plugin-opt=jobs=">, Alias<thinlto_jobs>, HelpText<"Alias for --thinlto-jobs">;
 def: J<"plugin-opt=lto-partitions=">, Alias<lto_partitions>, HelpText<"Alias for --lto-partitions">;
 def plugin_opt_mcpu_eq: J<"plugin-opt=mcpu=">;
 def: F<"plugin-opt=new-pass-manager">,
   Alias<lto_new_pass_manager>, HelpText<"Alias for --lto-new-pass-manager">;
 def: F<"plugin-opt=cs-profile-generate">,
   Alias<lto_cs_profile_generate>, HelpText<"Alias for --lto-cs-profile-generate">;
 def: J<"plugin-opt=cs-profile-path=">,
   Alias<lto_cs_profile_file>, HelpText<"Alias for --lto-cs-profile-file">;
 def: J<"plugin-opt=obj-path=">,

lld/test/ELF/lto/emit-asm.ll

+; REQUIRES: x86
+; RUN: llvm-as %s -o %t.o
+; RUN: ld.lld --lto-emit-asm -shared %t.o -o - | FileCheck %s
+; RUN: ld.lld --plugin-opt=emit-asm --plugin-opt=lto-partitions=2 -shared %t.o -o %t2.s
+; RUN: cat %t2.s %t2.s1 | FileCheck %s
+
+; RUN: ld.lld --lto-emit-asm --save-temps -shared %t.o -o %t3.s
+; RUN: FileCheck --input-file %t3.s %s
+; RUN: llvm-dis %t3.s.0.4.opt.bc -o - | FileCheck --check-prefix=OPT %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK-DAG: f1:
+; OPT-DAG: define void @f1()
+define void @f1() {
+  ret void
+}
+
+; CHECK-DAG: f2:
+; OPT-DAG: define void @f2()
+define void @f2() {
+  ret void
+}