[llvm-dev] libLTO Codegen options issue

[Wael Yehia via llvm-dev]

Thanks Steven.

>A quick feedback will be that (1) is not an option. libLTO APIs need
>to be stable and existing APIs cannot be changed.
Fair point. 


>I am curious about your motivation for the change. If you have access
>to `InitTargetOptionsFromCodeGenFlags`, then you don't need libLTO
>interface at all since you are building again LLVM and you are free
>to call any underlying API you want. 

InitTargetOptionsFromCodeGenFlags is used in the implementation of libLTO, and the implementation (not the interface) has access to llvm.
Does that clarify things?

>In libLTO, `parseCodeGenDebugOptions` is a debug function as name
>suggested. People should not rely on that in production. Instead, new
>APIs should be created for the underlying setting you need.
> 

The function comments indicate otherwise:
$ git show d5268ebe1925:llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h
    ...
    120   /// Pass options to the driver and optimization passes.
    121   ///
    122   /// These options are not necessarily for debugging purpose (the function
    123   /// name is misleading).  This function should be called before
    124   /// LTOCodeGenerator::compilexxx(), and
    125   /// LTOCodeGenerator::writeMergedModules().
    126   void setCodeGenDebugOptions(ArrayRef<StringRef> Opts);
    127 
    128   /// Parse the options set in setCodeGenDebugOptions.
    129   ///
    130   /// Like \a setCodeGenDebugOptions(), this must be called before
    131   /// LTOCodeGenerator::compilexxx() and
    132   /// LTOCodeGenerator::writeMergedModules().
    133   void parseCodeGenDebugOptions();

> I don't see reasons why you need to parse CodeGen flags before creating code
> generator.
As I mention in my email: 
  1. the construction of `llvm::TargetOptions` relies on having parsed the options
  2. the construction of LTOModule relies on the TargetOptions,
  3. the LTOCodeGenerator does not rely on TargetOptions, but the factory method `createCodeGen` which is used in libLTO to construct the opaque type lto_code_gen_t (which is just a pointer to a LTOCodeGenerator), creates a the LTOCodeGenerator and then sets the target options:
$ git show d5268ebe1925:llvm/tools/lto/lto.cpp
    ...
    363 static lto_code_gen_t createCodeGen(bool InLocalContext) {
    364   lto_initialize();
    365 
    366   TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags(Triple());
    367 
    368   LibLTOCodeGenerator *CodeGen =
    369       InLocalContext ? new LibLTOCodeGenerator(std::make_unique<LLVMContext>())
    370                      : new LibLTOCodeGenerator();
    371   CodeGen->setTargetOptions(Options);
    372   return wrap(CodeGen);
    373 }
    374 
    375 lto_code_gen_t lto_codegen_create(void) {
    376   return createCodeGen(/* InLocalContext */ false);
    377 }


I could move the creation of TargetOptions and CodeGen->setTargetOptions(Options) into:
    435 static void maybeParseOptions(lto_code_gen_t cg) {
    436   if (!parsedOptions) {
    437     unwrap(cg)->parseCodeGenDebugOptions();
    438     lto_add_attrs(cg);
    439     parsedOptions = true;
    440   }
    441 }

but maybeParseOptions is only called from compile and optimize functions (i.e. after we've read in all the LTO modules), so LTOModules would have inaccurate TargetOptions (because command line options have not been parsed yet).

Wael

-----Steven Wu <stevenwu at apple.com> wrote: -----

>To: Wael Yehia <wyehia at ca.ibm.com>
>From: Steven Wu <stevenwu at apple.com>
>Date: 12/03/2020 06:11PM
>Cc: llvm-dev at lists.llvm.org, joker.eph at gmail.com
>Subject: [EXTERNAL] Re: libLTO Codegen options issue
>
>A quick feedback will be that (1) is not an option. libLTO APIs need
>to be stable and existing APIs cannot be changed.
>
>I am curious about your motivation for the change. If you have access
>to `InitTargetOptionsFromCodeGenFlags`, then you don't need libLTO
>interface at all since you are building again LLVM and you are free
>to call any underlying API you want. 
>
>In libLTO, `parseCodeGenDebugOptions` is a debug function as name
>suggested. People should not rely on that in production. Instead, new
>APIs should be created for the underlying setting you need. I don't
>see reasons why you need to parse CodeGen flags before creating code
>generator.
>
>Steven
>
>> On Dec 3, 2020, at 2:57 PM, Wael Yehia <wyehia at ca.ibm.com> wrote:
>> 
>> Hi,
>> We're trying to use libLTO, and noticed an issue (more of a timing
>problem) with option processing where TargetOptions is created before
>cl::ParseCommandLineOptions is invoked.
>> Right now, the only place where 
>>  ParseCommandLineOptions is called is in
>LTOCodeGenerator::parseCodeGenDebugOptions, 
>>  which is only called from maybeParseOptions,
>>  which is called after TargetOptions have been created.
>> We construct TargetOptions (by calling
>InitTargetOptionsFromCodeGenFlags) first and then use it to we create
>modules or the codegen object.
>> 
>> Assuming this is in fact a problem, one way to fix this is to
>decouple parseCodeGenDebugOptions from LTOCodeGenerator, so that it
>can be called before we create the LTOCodeGenerator.
>> 
>> Now we can either 
>>   (1) modify the signature of the lto_codegen_debug_options and
>lto_codegen_debug_options_array API functions 
>> or (2) add new API functions.
>> 
>> I prefer (1) because as it is now, the API is broken.
>> I uploaded a patch here
>INVALID URI REMOVED
>_D92611&d=DwIFAg&c=jf_iaSHvJObTbx-siA1ZOg&r=z6IPFss9EigepE3DNR_kfA15n
>rHJhAw6dnvEXz_GHvU&m=nxuUhu_XwX2i7vXVLKxoBKtX2GQDqBVZvkN7o0m9ltE&s=Gx
>m0FzQUCAbsP-vv5nMMZCURqJje6Ft2T_t1PEmnnbY&e= 
>> 
>> Any feedback is appreciated. Thank you
>> 
>> Wael
>> 
>
>