in this patch we add a new libLTO API to specify debug options independent of an lto_code_gen_t.
This allows clients to pass codegen flags (through libLTO) which otherwise today are ignored.
Details
Diff Detail
Event Timeline
Feedback has been provided on the mailing list. Using function attributes is preferred and changing legacy C API is not allowed.
in this patch we add a new libLTO API to specify debug options independent of an lto_code_gen_t.
This allows clients to pass codegen flags (through libLTO) which otherwise today are ignored.
Like replied in email. The non-scaleable option is very intentional and debug_option is by its name for compiler developer only and should not be used in production. The preferred option is to create API to set every single attributes you want to set.
llvm/tools/lto/lto.cpp | ||
---|---|---|
466 | If you have to pick an interface, use the new _array interface. This is also not correct since you skipped lto_add_attrs function when you called your version. |
llvm/tools/lto/lto.cpp | ||
---|---|---|
466 |
I might be missing something. I think some function and variable names could be improved to reflect their intended purpose. |
changes made:
- use _array interface in lto_codegen_debug_options_early
- replace parsedOptionsEarly and parsedOptions with a single enum variable.
I still don't like this implementation, but fine if you have to do this.
But please rename the function, this is no longer a lto_codegen function. Just named it something like lto_set_debug_options, which is also cleaner.
llvm/tools/lto/lto.cpp | ||
---|---|---|
426 | Shouldn't this be: if (optionParsingState == OptParsingState::NotParsed) unwrap(cg)->parseCodeGenDebugOptions(); if (optionParsingState != OptParsingState::Done) lto_add_attrs(cg); optionParsingState = OptParsingState::Done; |
llvm/tools/lto/lto.cpp | ||
---|---|---|
426 | 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. |
renamed lto_codegen_debug_options_early to lto_set_debug_options, and added an entry to lto.exports
llvm/tools/lto/lto.cpp | ||
---|---|---|
426 | 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. |
Can you just revert to use parsedOptions as boolean? The state you used right now doesn't add anything. You can just leave lto_set_debug_options not guarded right?
I consider the two APIs lto_set_debug_options and lto_codegen_debug_options to be mutually exclusive, so I need to keep track whether lto_set_debug_options was called.
Before creating the enum state, I had two booleans parsedOptions and parsedOptionsEarly where parsedOptionsEarly is a boolean I added to tell that early options has been set (i.e. lto_set_debug_options called).
However, I cannot set parsedOptions when my new API function is called because I want maybeParseOptions to still do it's thing. So I needed that second boolean.
So I then created the state variable in place of the two booleans (cleaner in my opinion and avoids having to check combinations of the two booleans) to tell which state we're in:
NotParsed, // Initial state. before either lto_set_debug_options or maybeParseOptions is called. Early, // After lto_set_debug_options is called. This state will be skipped if user calls lto_codegen_debug_options. Done // After maybeParseOptions is called (i.e. unwrap(cg)->parseCodeGenDebugOptions() and lto_add_attrs(cg) have executed)
The state can either change NotParsed -> Done or NotParsed -> Early -> Done depending on which API was used to set debug options.
Is there a problem if both APIs are called (providing they don't conflict with each other or duplicate arguments?)
Is there a problem if both APIs are called (providing they don't conflict with each other or duplicate arguments?)
If you call both APIs, then you run cl::ParseCommandLineOptions twice. I haven't tested it, but I'm pretty sure it's not intended to be called twice.
clang-tidy: warning: invalid case style for function 'lto_codegen_debug_options_early' [readability-identifier-naming]
not useful
clang-tidy: warning: invalid case style for parameter 'options' [readability-identifier-naming]
not useful