[LTO][Legacy] Decouple option parsing from LTOCodeGenerator

[w2yehia]

Summary

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.

[steven_wu]

Feedback has been provided on the mailing list. Using function attributes is preferred and changing legacy C API is not allowed.

[w2yehia]

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.

[steven_wu]

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

468

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.

[w2yehia]

llvm/tools/lto/lto.cpp

468

This is also not correct since you skipped lto_add_attrs function when you called your version.

I might be missing something.
lto_add_attrs will still run where it usually runs, in maybeParseOptions.
Note that I introduced a new boolean, parsedOptionsEarly, to indicate whether early parsing happens.
Also, I kept the code in maybeParseOptions untouched, so when we call unwrap(cg)->parseCodeGenDebugOptions() no parsing would happen if we haven't called unwrap(cg)->setCodeGenDebugOptions earlier.

I think some function and variable names could be improved to reflect their intended purpose.
For example, LTOCodeGenerator::parseCodeGenDebugOptions ==> LTOCodeGenerator::parseSavedCodeGenDebugOptions.
Also I can add a LTOCodeGenerator::postOptionParsing function, to contain code such as the save-temps registration.

[w2yehia]

changes made:

• use _array interface in lto_codegen_debug_options_early
• replace parsedOptionsEarly and parsedOptions with a single enum variable.

[w2yehia]

@steven_wu I've addressed your comment. Please review when you have a chance.

[steven_wu]

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;

[w2yehia]

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.
I'll let you decide which way we go.
Thanks

[w2yehia]

renamed lto_codegen_debug_options_early to lto_set_debug_options, and added an entry to lto.exports

[w2yehia]

@steven_wu please take a look when you have a chance. Thanks

[w2yehia]

@steven_wu ping. thanks

[steven_wu]

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.

[w2yehia]

@steven_wu thanks for your review. Is this good to be merged then?

[steven_wu]

In D92611#2659079, @w2yehia wrote:

@steven_wu thanks for your review. Is this good to be merged then?

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?

[w2yehia]

In D92611#2659437, @steven_wu wrote:

In D92611#2659079, @w2yehia wrote:

@steven_wu thanks for your review. Is this good to be merged then?

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.

[steven_wu]

In D92611#2659603, @w2yehia wrote:

In D92611#2659437, @steven_wu wrote:

In D92611#2659079, @w2yehia wrote:

@steven_wu thanks for your review. Is this good to be merged then?

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?)

[w2yehia]

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.

[steven_wu]

Ok, fine with that.

[w2yehia]

Thanks @steven_wu

llvm/include/llvm-c/lto.h

 
 /**
  * @defgroup LLVMCLTO LTO
  * @ingroup LLVMC
  *
  * @{
  */
 
-#define LTO_API_VERSION 27
+#define LTO_API_VERSION 28
 
 /**
  * \since prior to LTO_API_VERSION=3
  */
 typedef enum {
     LTO_SYMBOL_ALIGNMENT_MASK              = 0x0000001F, /* log2 of alignment */
     LTO_SYMBOL_PERMISSIONS_MASK            = 0x000000E0,
     LTO_SYMBOL_PERMISSIONS_CODE            = 0x000000A0,

  * Returns the runtime API version.
  *
  * \since LTO_API_VERSION=12
  */
 extern unsigned int
 lto_api_version(void);
 
 /**
- * Sets options to help debug codegen bugs.
+ * Parses options immediately, making them available as early as possible. For
+ * example during executing codegen::InitTargetOptionsFromCodeGenFlags. Since
+ * parsing shud only happen once, only one of lto_codegen_debug_options or
+ * lto_set_debug_options should be called.
+ *
+ * This function takes one or more options separated by spaces.
+ * Warning: passing file paths through this function may confuse the argument
+ * parser if the paths contain spaces.
+ *
+ * \since LTO_API_VERSION=28
+ */
+extern void lto_set_debug_options(const char *const *options, int number);
+
+/**
+ * Sets options to help debug codegen bugs. Since parsing shud only happen once,
+ * only one of lto_codegen_debug_options or lto_set_debug_options
+ * should be called.
  *
  * This function takes one or more options separated by spaces.
  * Warning: passing file paths through this function may confuse the argument
  * parser if the paths contain spaces.
  *
  * \since prior to LTO_API_VERSION=3
  */
 extern void

llvm/include/llvm/LTO/legacy/LTOCodeGenerator.h

private:
   bool ShouldInternalize = EnableLTOInternalization;
   bool ShouldEmbedUselists = false;
   bool ShouldRestoreGlobalsLinkage = false;
   std::unique_ptr<ToolOutputFile> DiagnosticOutputFile;
   std::unique_ptr<ToolOutputFile> StatsFile = nullptr;
 
   lto::Config Config;
 };
+
+/// A convenience function that calls cl::ParseCommandLineOptions on the given
+/// set of options.
+void parseCommandLineOptions(std::vector<std::string> &Options);
 }
 #endif

llvm/lib/LTO/LTOCodeGenerator.cpp

 }
 
 void LTOCodeGenerator::setCodeGenDebugOptions(ArrayRef<StringRef> Options) {
   for (StringRef Option : Options)
     CodegenOptions.push_back(Option.str());
 }
 
 void LTOCodeGenerator::parseCodeGenDebugOptions() {
-  // if options were requested, set them
-  if (!CodegenOptions.empty()) {
+  if (!CodegenOptions.empty())
+    llvm::parseCommandLineOptions(CodegenOptions);
+}
+
+void llvm::parseCommandLineOptions(std::vector<std::string> &Options) {
+  if (!Options.empty()) {
     // ParseCommandLineOptions() expects argv[0] to be program name.
     std::vector<const char *> CodegenArgv(1, "libLLVMLTO");
-    for (std::string &Arg : CodegenOptions)
+    for (std::string &Arg : Options)
       CodegenArgv.push_back(Arg.c_str());
     cl::ParseCommandLineOptions(CodegenArgv.size(), CodegenArgv.data());
   }
 }
 
-
 void LTOCodeGenerator::DiagnosticHandler(const DiagnosticInfo &DI) {
   // Map the LLVM internal diagnostic severity to the LTO diagnostic severity.
   lto_codegen_diagnostic_severity_t Severity;
   switch (DI.getSeverity()) {
   case DS_Error:
     Severity = LTO_DS_ERROR;
     break;
   case DS_Warning:

llvm/tools/lto/lto.cpp

 // Holds most recent error string.
 // *** Not thread safe ***
 static std::string sLastErrorString;
 
 // Holds the initialization state of the LTO module.
 // *** Not thread safe ***
 static bool initialized = false;
 
-// Holds the command-line option parsing state of the LTO module.
-static bool parsedOptions = false;
+// Represent the state of parsing command line debug options.
+static enum class OptParsingState {
+  NotParsed, // Initial state.
+  Early,     // After lto_set_debug_options is called.
+  Done       // After maybeParseOptions is called.
+} optionParsingState = OptParsingState::NotParsed;
 
 static LLVMContext *LTOContext = nullptr;
 
 struct LTOToolDiagnosticHandler : public DiagnosticHandler {
   bool handleDiagnostics(const DiagnosticInfo &DI) override {
     if (DI.getSeverity() != DS_Error) {
       DiagnosticPrinterRawOStream DP(errs());
       DI.print(DP);

 }
 
 void lto_codegen_add_must_preserve_symbol(lto_code_gen_t cg,
                                           const char *symbol) {
   unwrap(cg)->addMustPreserveSymbol(symbol);
 }
 
 static void maybeParseOptions(lto_code_gen_t cg) {
-  if (!parsedOptions) {
+  if (optionParsingState != OptParsingState::Done) {
+    // Parse options if any were set by the lto_codegen_debug_options* function.
     unwrap(cg)->parseCodeGenDebugOptions();
     lto_add_attrs(cg);
-    parsedOptions = true;
+    optionParsingState = OptParsingState::Done;
   }
 }
 
 bool lto_codegen_write_merged_modules(lto_code_gen_t cg, const char *path) {
   maybeParseOptions(cg);
   return !unwrap(cg)->writeMergedModules(path);
 }
 
const void *lto_codegen_compile_optimized(lto_code_gen_t cg, size_t *length) {
   return CG->NativeObjectFile->getBufferStart();
 }
 
 bool lto_codegen_compile_to_file(lto_code_gen_t cg, const char **name) {
   maybeParseOptions(cg);
   return !unwrap(cg)->compile_to_file(name);
 }
 
+void lto_set_debug_options(const char *const *options, int number) {
+  assert(optionParsingState == OptParsingState::NotParsed &&
+         "option processing already happened");
+  // Need to put each suboption in a null-terminated string before passing to
+  // parseCommandLineOptions().
+  std::vector<std::string> Options;
+  for (int i = 0; i < number; ++i)
+    Options.push_back(options[i]);
+
+  llvm::parseCommandLineOptions(Options);
+  optionParsingState = OptParsingState::Early;
+}
+
 void lto_codegen_debug_options(lto_code_gen_t cg, const char *opt) {
+  assert(optionParsingState != OptParsingState::Early &&
+         "early option processing already happened");
   SmallVector<StringRef, 4> Options;
   for (std::pair<StringRef, StringRef> o = getToken(opt); !o.first.empty();
        o = getToken(o.second))
     Options.push_back(o.first);
 
   unwrap(cg)->setCodeGenDebugOptions(Options);
 }
 
 void lto_codegen_debug_options_array(lto_code_gen_t cg,
                                      const char *const *options, int number) {
+  assert(optionParsingState != OptParsingState::Early &&
+         "early option processing already happened");
   SmallVector<StringRef, 4> Options;
   for (int i = 0; i < number; ++i)
     Options.push_back(options[i]);
   unwrap(cg)->setCodeGenDebugOptions(makeArrayRef(Options));
 }
 
 unsigned int lto_api_version() { return LTO_API_VERSION; }
 

llvm/tools/lto/lto.exports

 lto_codegen_set_assembler_args
 lto_codegen_set_assembler_path
 lto_codegen_set_cpu
 lto_codegen_compile_to_file
 lto_codegen_optimize
 lto_codegen_compile_optimized
 lto_codegen_set_should_internalize
 lto_codegen_set_should_embed_uselists
+lto_set_debug_options
 LLVMCreateDisasm
 LLVMCreateDisasmCPU
 LLVMDisasmDispose
 LLVMDisasmInstruction
 LLVMSetDisasmOptions
 LLVMCreateDisasmCPUFeatures
 thinlto_create_codegen
 thinlto_codegen_dispose