<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Dec 3, 2020 at 4:14 PM Wael Yehia <<a href="mailto:wyehia@ca.ibm.com">wyehia@ca.ibm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Thanks Steven.<br>
<br>
>A quick feedback will be that (1) is not an option. libLTO APIs need<br>
>to be stable and existing APIs cannot be changed.<br>
Fair point. <br>
<br>
<br>
>I am curious about your motivation for the change. If you have access<br>
>to `InitTargetOptionsFromCodeGenFlags`, then you don't need libLTO<br>
>interface at all since you are building again LLVM and you are free<br>
>to call any underlying API you want. <br>
<br>
InitTargetOptionsFromCodeGenFlags is used in the implementation of libLTO, and the implementation (not the interface) has access to llvm.<br>
Does that clarify things?<br>
<br>
>In libLTO, `parseCodeGenDebugOptions` is a debug function as name<br>
>suggested. People should not rely on that in production. Instead, new<br>
>APIs should be created for the underlying setting you need.<br>
> <br>
<br>
The function comments indicate otherwise:<br>
$ git show d5268ebe1925:llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
...<br>
120 /// Pass options to the driver and optimization passes.<br>
121 ///<br>
122 /// These options are not necessarily for debugging purpose (the function<br>
123 /// name is misleading). This function should be called before<br>
124 /// LTOCodeGenerator::compilexxx(), and<br>
125 /// LTOCodeGenerator::writeMergedModules().<br>
126 void setCodeGenDebugOptions(ArrayRef<StringRef> Opts);<br>
127 <br>
128 /// Parse the options set in setCodeGenDebugOptions.<br>
129 ///<br>
130 /// Like \a setCodeGenDebugOptions(), this must be called before<br>
131 /// LTOCodeGenerator::compilexxx() and<br>
132 /// LTOCodeGenerator::writeMergedModules().<br>
133 void parseCodeGenDebugOptions();<br></blockquote><div><br></div><div><br></div><div>In general, I see any cl::opt as a debugging tool. Clients should use programmatic API and not rely on string-based CL opt to control what's happening behind the APIs.</div><div><br></div><div>-- </div><div>Mehdi</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
> I don't see reasons why you need to parse CodeGen flags before creating code<br>
> generator.<br>
As I mention in my email: <br>
1. the construction of `llvm::TargetOptions` relies on having parsed the options<br>
2. the construction of LTOModule relies on the TargetOptions,<br>
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:<br>
$ git show d5268ebe1925:llvm/tools/lto/lto.cpp<br>
...<br>
363 static lto_code_gen_t createCodeGen(bool InLocalContext) {<br>
364 lto_initialize();<br>
365 <br>
366 TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags(Triple());<br>
367 <br>
368 LibLTOCodeGenerator *CodeGen =<br>
369 InLocalContext ? new LibLTOCodeGenerator(std::make_unique<LLVMContext>())<br>
370 : new LibLTOCodeGenerator();<br>
371 CodeGen->setTargetOptions(Options);<br>
372 return wrap(CodeGen);<br>
373 }<br>
374 <br>
375 lto_code_gen_t lto_codegen_create(void) {<br>
376 return createCodeGen(/* InLocalContext */ false);<br>
377 }<br>
<br>
<br>
I could move the creation of TargetOptions and CodeGen->setTargetOptions(Options) into:<br>
435 static void maybeParseOptions(lto_code_gen_t cg) {<br>
436 if (!parsedOptions) {<br>
437 unwrap(cg)->parseCodeGenDebugOptions();<br>
438 lto_add_attrs(cg);<br>
439 parsedOptions = true;<br>
440 }<br>
441 }<br>
<br>
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).<br>
<br>
Wael<br>
<br>
-----Steven Wu <<a href="mailto:stevenwu@apple.com" target="_blank">stevenwu@apple.com</a>> wrote: -----<br>
<br>
>To: Wael Yehia <<a href="mailto:wyehia@ca.ibm.com" target="_blank">wyehia@ca.ibm.com</a>><br>
>From: Steven Wu <<a href="mailto:stevenwu@apple.com" target="_blank">stevenwu@apple.com</a>><br>
>Date: 12/03/2020 06:11PM<br>
>Cc: <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>, <a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a><br>
>Subject: [EXTERNAL] Re: libLTO Codegen options issue<br>
><br>
>A quick feedback will be that (1) is not an option. libLTO APIs need<br>
>to be stable and existing APIs cannot be changed.<br>
><br>
>I am curious about your motivation for the change. If you have access<br>
>to `InitTargetOptionsFromCodeGenFlags`, then you don't need libLTO<br>
>interface at all since you are building again LLVM and you are free<br>
>to call any underlying API you want. <br>
><br>
>In libLTO, `parseCodeGenDebugOptions` is a debug function as name<br>
>suggested. People should not rely on that in production. Instead, new<br>
>APIs should be created for the underlying setting you need. I don't<br>
>see reasons why you need to parse CodeGen flags before creating code<br>
>generator.<br>
><br>
>Steven<br>
><br>
>> On Dec 3, 2020, at 2:57 PM, Wael Yehia <<a href="mailto:wyehia@ca.ibm.com" target="_blank">wyehia@ca.ibm.com</a>> wrote:<br>
>> <br>
>> Hi,<br>
>> We're trying to use libLTO, and noticed an issue (more of a timing<br>
>problem) with option processing where TargetOptions is created before<br>
>cl::ParseCommandLineOptions is invoked.<br>
>> Right now, the only place where <br>
>> ParseCommandLineOptions is called is in<br>
>LTOCodeGenerator::parseCodeGenDebugOptions, <br>
>> which is only called from maybeParseOptions,<br>
>> which is called after TargetOptions have been created.<br>
>> We construct TargetOptions (by calling<br>
>InitTargetOptionsFromCodeGenFlags) first and then use it to we create<br>
>modules or the codegen object.<br>
>> <br>
>> Assuming this is in fact a problem, one way to fix this is to<br>
>decouple parseCodeGenDebugOptions from LTOCodeGenerator, so that it<br>
>can be called before we create the LTOCodeGenerator.<br>
>> <br>
>> Now we can either <br>
>> (1) modify the signature of the lto_codegen_debug_options and<br>
>lto_codegen_debug_options_array API functions <br>
>> or (2) add new API functions.<br>
>> <br>
>> I prefer (1) because as it is now, the API is broken.<br>
>> I uploaded a patch here<br>
>INVALID URI REMOVED<br>
>_D92611&d=DwIFAg&c=jf_iaSHvJObTbx-siA1ZOg&r=z6IPFss9EigepE3DNR_kfA15n<br>
>rHJhAw6dnvEXz_GHvU&m=nxuUhu_XwX2i7vXVLKxoBKtX2GQDqBVZvkN7o0m9ltE&s=Gx<br>
>m0FzQUCAbsP-vv5nMMZCURqJje6Ft2T_t1PEmnnbY&e= <br>
>> <br>
>> Any feedback is appreciated. Thank you<br>
>> <br>
>> Wael<br>
>> <br>
><br>
><br>
<br>
</blockquote></div></div>