libLTO Codegen options issue

Wael_Yehia1 · December 3, 2020, 10:57pm

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 https://reviews.llvm.org/D92611

Any feedback is appreciated. Thank you

Wael

cachemeifyoucan · December 3, 2020, 11:11pm

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

Wael_Yehia1 · December 4, 2020, 12:14am

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@apple.com> wrote: -----

mehdi_amini · December 4, 2020, 12:23am

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.

cachemeifyoucan · December 4, 2020, 12:28am

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 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();

We don’t officially support any option that has to pick up by cl::opt.

I don’t see reasons why you need to parse CodeGen flags before creating code
generator.

As I mention in my email:

the construction of llvm::TargetOptions relies on having parsed the options

the construction of LTOModule relies on the TargetOptions,

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())
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).

Module doesn’t have TargetOption. All it carries is the target triple and all the feature flags in the metadata. I would suggest you come with solid example about which cl::opt you are trying to overwrite and let’s see if we can make an API out of it.

Steven

Wael_Yehia1 · December 4, 2020, 2:17pm

Hi Steven and Mehdi,

Module doesn't have TargetOption. All it carries is the target triple
and all the feature flags in the metadata.

I was talking about LTOModule, whose factory method (LTOModule::makeLTOModule) takes a `TargetOptions` object.
It uses it to create a TargetMachine.

I would suggest you come
with solid example about which cl::opt you are trying to overwrite
and let's see if we can make an API out of it.

please see my reasoning in my next paragraph.

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.

I see, makes sense.
But what if the cl::opt, that is used for debugging only, is not respected when you're actually trying to use it during debugging?
This is the issue here, the mechanism by which you can pass "debugging" options to libLTO has a flaw where as a result, some cl::opt objects are read too soon, before option parsing has occurred.

To be specific, we were adding a cl::opt option to control the value of a field in a `TargetOptions` object when the object is created via InitTargetOptionsFromCodeGenFlags. This is nothing new, as there are already a bunch of options (for example -march, -mcpu, -relocation-model) in CommandFlags.cpp that have an effect on TargetOptions objects created via InitTargetOptionsFromCodeGenFlags (which libLTO currently calls)

Surely, we don't wanna add a new function to the libLTO API for every option in CommandFlag.cpp?

Also please note, that we as the client of libLTO are an intermediary between the real user and LTO. And the real user when debugging his application will try passing options to the LTO component that he sees are listed with --help, and so options defined in CommandFlag.cpp won't behave as expected.

Thanks everyone.

Wael

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

cachemeifyoucan · December 4, 2020, 5:17pm

Hi Steven and Mehdi,

Module doesn't have TargetOption. All it carries is the target triple
and all the feature flags in the metadata.

I was talking about LTOModule, whose factory method (LTOModule::makeLTOModule) takes a `TargetOptions` object.
It uses it to create a TargetMachine.

I would suggest you come
with solid example about which cl::opt you are trying to overwrite
and let's see if we can make an API out of it.

please see my reasoning in my next paragraph.

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.

I see, makes sense.
But what if the cl::opt, that is used for debugging only, is not respected when you're actually trying to use it during debugging?
This is the issue here, the mechanism by which you can pass "debugging" options to libLTO has a flaw where as a result, some cl::opt objects are read too soon, before option parsing has occurred.

To be specific, we were adding a cl::opt option to control the value of a field in a `TargetOptions` object when the object is created via InitTargetOptionsFromCodeGenFlags. This is nothing new, as there are already a bunch of options (for example -march, -mcpu, -relocation-model) in CommandFlags.cpp that have an effect on TargetOptions objects created via InitTargetOptionsFromCodeGenFlags (which libLTO currently calls)

Surely, we don't wanna add a new function to the libLTO API for every option in CommandFlag.cpp?

Do you have a specific option in TargetOption in mind or it is something only in your downstream?

The idea is that modules need to be standalone and contain all the information needed to construct the target it is optimized for. Most of those target specific options cannot be overwrite during link time because the previous optimization pipeline might have assumed otherwise. The preferred way to set a target related option is to put that into a function attribute. E.g.:

attributes #0 = { "target-features"="+foo" }

You just need to teach code generator to correctly understand them.

Steven

Wael_Yehia1 · March 17, 2021, 4:00am

Hi Steven,

Do you have a specific option in TargetOption in mind or it is something only in your downstream?

function-sections is one example.
- It's a codegen flag that has no corresponding attribute in the IR (in ⚙ D24644 Pass -ffunction-sections/-fdata-sections along to gold-plugin there was consideration to add an attribute).
- Clang currently passes -plugin-opt=-function-sections to the linker with -flto and -ffunction-sections.
- There's no libLTO API to control/set the flag.
- the option is respected by LLVMgold.so (via the cl::opt options queried in codegen::InitTargetOptionsFromCodeGenFlags), and is ignore by lld.

1. what is the recommended approach to fix this in libLTO?
2. The timing problem I described in my initial email is still present, and fixing it would aid in debugging libLTO codegen issues at the least. I'll update ⚙ D92611 [LTO][Legacy] Decouple option parsing from LTOCodeGenerator with a fix that adds a new API for "early" debug option processing.

Wael Yehia
Compiler Development
IBM Canada Lab
wyehia@ca.ibm.com

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

cachemeifyoucan · March 17, 2021, 9:46pm

Hi Steven,

Do you have a specific option in TargetOption in mind or it is something only in your downstream?

function-sections is one example.
- It's a codegen flag that has no corresponding attribute in the IR (in ⚙ D24644 Pass -ffunction-sections/-fdata-sections along to gold-plugin there was consideration to add an attribute).
- Clang currently passes -plugin-opt=-function-sections to the linker with -flto and -ffunction-sections.
- There's no libLTO API to control/set the flag.
- the option is respected by LLVMgold.so (via the cl::opt options queried in codegen::InitTargetOptionsFromCodeGenFlags), and is ignore by lld.

libLTO has nothing to do with lld. libLTO provides the c API to be used by other linkers which are not in llvm-project.

1. what is the recommended approach to fix this in libLTO?

For function section, if not used a attribute, just add a new API to libLTO that set function section in the target machine.

Steven

Wael_Yehia1 · March 17, 2021, 10:22pm

Thanks Steven.

libLTO has nothing to do with lld. libLTO provides the c API to be used by other linkers which are not in llvm-project.

Our use case is we have a linker that we want to teach to do LTO on llvm files and basically be as close to a replacement (in terms of the user/options inteface) to lld and ld+LLVMGold as possible.
So I'm mentioning lld's behaviour for reference and comparison, since both lld and the external linker are invoked by clang, and it wud be ideal that clang emits the same set of linker flags (when possible).

For function section, if not used a attribute, just add a new API to libLTO that set function section in the target machine.

Yea, that's the obvious fix, but I'm not sure its scalable (if other flags are needed).

Would it be useful to have a function/class that takes in a list of plugin-opt options (input format TBD) and either updates or creates a new llvm::Config object.
This function/class could be shared by gold-plugin and lld, and can be made available in the libLTO api for external linkers?
We don't have it ready, just brainstorming..

Wael Yehia
Compiler Development
IBM Canada Lab
wyehia@ca.ibm.com

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