[PATCH] D77231: [lld] Support -emit-asm with LTO
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 2 01:36:00 PDT 2020
grimar added inline comments.
================
Comment at: lld/ELF/Driver.cpp:890
config->emitLLVM = args.hasArg(OPT_plugin_opt_emit_llvm, false);
+ config->emitAsm = args.hasArg(OPT_lto_emit_asm, false);
config->emitRelocs = args.hasArg(OPT_emit_relocs);
----------------
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?
================
Comment at: lld/ELF/Driver.cpp:1937
+ if (config->emitAsm)
+ return;
+
----------------
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;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77231/new/
https://reviews.llvm.org/D77231
More information about the llvm-commits
mailing list