[PATCH] D92611: [LTO][Legacy] Decouple option parsing from LTOCodeGenerator

Steven Wu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 30 09:31:35 PDT 2021


steven_wu added inline comments.


================
Comment at: llvm/tools/lto/lto.cpp:426
+  if (optionParsingState != OptParsingState::Done) {
+    // Parse options if any were set by the lto_codegen_debug_options* function.
     unwrap(cg)->parseCodeGenDebugOptions();
----------------
w2yehia wrote:
> steven_wu wrote:
> > Shouldn't this be:
> > ```
> > if (optionParsingState ==  OptParsingState::NotParsed)
> >   unwrap(cg)->parseCodeGenDebugOptions();
> > 
> > if (optionParsingState !=  OptParsingState::Done)
> >     lto_add_attrs(cg);
> > 
> > optionParsingState = OptParsingState::Done;
> > ```
> I considered that... the only disadvantage with guarding `unwrap(cg)->parseCodeGenDebugOptions();` is that now it's not going to run when early option parsing occurs and we would need an additional function in LTOCodeGenerator to contain code that we want to execute **after** debug option parsing has occurred. For example the code to register save-temps (that D98183 is planning to add).
> 
> Other than that I think your suggestion is abit more intuitive when reading the code.
> I'll let you decide which way we go.
> Thanks
D98183 is really a debug option which is not used for production and it doesn't matter when you parse that option.

Actually thinking about this again, maybe keep this as it was if the command line can just be parsed twice. The way you implement it makes two options exclusive. If you use your parsing function, lto_add_attrs cannot be reached so you lost some codegen options.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92611/new/

https://reviews.llvm.org/D92611



More information about the llvm-commits mailing list