Page MenuHomePhabricator

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

Authored by hoyFB on Apr 1 2020, 10:25 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MaskRay added inline comments.Apr 1 2020, 11:45 AM
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 edited the summary of this revision. (Show Details)Apr 1 2020, 12:01 PM
hoyFB marked 2 inline comments as done.
hoyFB added inline comments.
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 marked an inline comment as not done.Apr 1 2020, 12:14 PM
hoyFB updated this revision to Diff 254275.Apr 1 2020, 12:47 PM

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

grimar added inline comments.Apr 2 2020, 1:11 AM
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 added inline comments.Apr 2 2020, 1:13 AM
lld/ELF/Driver.cpp
1963

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

hoyFB marked 3 inline comments as done.Apr 2 2020, 9:16 AM
hoyFB added inline comments.
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 marked an inline comment as done.Apr 2 2020, 9:24 AM
hoyFB added inline comments.
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 added a subscriber: pcc.Apr 2 2020, 11:48 AM
MaskRay added inline comments.
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 marked an inline comment as done.Apr 2 2020, 12:10 PM
hoyFB added inline comments.
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 added inline comments.Apr 2 2020, 12:10 PM
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 updated this revision to Diff 254592.Apr 2 2020, 12:18 PM

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

hoyFB marked 4 inline comments as done.Apr 2 2020, 12:30 PM
grimar added inline comments.Apr 3 2020, 1:46 AM
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 added a comment.Apr 13 2020, 9:24 AM

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

MaskRay retitled this revision from [lld] Support -emit-asm with LTO to [lld] Support --lto-emit-asm and --plugin-opt=emit-asm.Apr 13 2020, 12:07 PM

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 updated this revision to Diff 257104.Apr 13 2020, 1:46 PM
hoyFB marked 2 inline comments as done.

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 added inline comments.Apr 13 2020, 2:20 PM
lld/ELF/Driver.cpp
920

Delete , false which is redundant

hoyFB updated this revision to Diff 257125.Apr 13 2020, 3:06 PM

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

hoyFB marked an inline comment as done.Apr 13 2020, 3:06 PM
MaskRay added inline comments.Apr 13 2020, 3:21 PM
lld/ELF/Options.td
544

This is not done.

hoyFB marked an inline comment as done.Apr 13 2020, 3:25 PM
hoyFB added inline comments.
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 added inline comments.Apr 13 2020, 3:52 PM
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 updated this revision to Diff 257135.Apr 13 2020, 3:59 PM
hoyFB marked an inline comment as done.

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

lld/ELF/Options.td
544

Gotcha, thanks for the explanation.

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

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 marked an inline comment as done.Apr 16 2020, 1:30 PM
hoyFB added inline comments.
lld/test/ELF/lto/emit-asm.ll
2

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

hoyFB updated this revision to Diff 258164.Apr 16 2020, 1:55 PM
hoyFB marked 2 inline comments as done.

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

hoyFB marked 2 inline comments as done.Apr 16 2020, 1:56 PM
hoyFB updated this revision to Diff 259111.Apr 21 2020, 3:11 PM

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

MaskRay added inline comments.Apr 21 2020, 3:51 PM
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 added inline comments.Apr 21 2020, 3:52 PM
lld/ELF/Options.td
543

Move it before emit_llvm for local lexicographical order.

@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 updated this revision to Diff 260025.Apr 24 2020, 4:24 PM
hoyFB marked 3 inline comments as done.

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

hoyFB added a comment.Apr 24 2020, 4:25 PM

@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 updated this revision to Diff 260026.Apr 24 2020, 4:29 PM

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

hoyFB marked an inline comment as done.Apr 24 2020, 4:29 PM
MaskRay accepted this revision.Apr 24 2020, 5:00 PM

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

This revision is now accepted and ready to land.Apr 24 2020, 5:00 PM
Harbormaster completed remote builds in B54643: Diff 260025.
grimar accepted this revision.Apr 27 2020, 12:50 AM

LGTM

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

This revision was automatically updated to reflect the committed changes.

@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 -
}

@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?

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