Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D92611 Diff 331729 llvm/tools/lto/lto.cpp

Changeset View

Standalone View

View Options

llvm/tools/lto/lto.cpp

Show First 20 Lines • Show All 60 Lines • ▼ Show 20 Lines
// Holds the initialization state of the LTO module. // Holds the initialization state of the LTO module.
// *** Not thread safe *** // *** Not thread safe ***
static bool initialized = false; static bool initialized = false;
// Holds the command-line option parsing state of the LTO module. // Holds the command-line option parsing state of the LTO module.
static bool parsedOptions = false; static bool parsedOptions = false;
// Set to true if options were parsed early.
static bool parsedOptionsEarly = false;
Lint: Pre-merge checks
Inline Actions

clang-tidy: warning: invalid case style for variable 'parsedOptionsEarly' [readability-identifier-naming]
not useful

Lint: Pre-merge checks: clang-tidy: warning: invalid case style for variable 'parsedOptionsEarly' [readability…
static LLVMContext *LTOContext = nullptr; static LLVMContext *LTOContext = nullptr;
struct LTOToolDiagnosticHandler : public DiagnosticHandler { struct LTOToolDiagnosticHandler : public DiagnosticHandler {
bool handleDiagnostics(const DiagnosticInfo &DI) override { bool handleDiagnostics(const DiagnosticInfo &DI) override {
if (DI.getSeverity() != DS_Error) { if (DI.getSeverity() != DS_Error) {
DiagnosticPrinterRawOStream DP(errs()); DiagnosticPrinterRawOStream DP(errs());
DI.print(DP); DI.print(DP);
errs() << '\n'; errs() << '\n';
▲ Show 20 Lines • Show All 338 Lines • ▼ Show 20 Lines
void lto_codegen_add_must_preserve_symbol(lto_code_gen_t cg, void lto_codegen_add_must_preserve_symbol(lto_code_gen_t cg,
const char *symbol) { const char *symbol) {
unwrap(cg)->addMustPreserveSymbol(symbol); unwrap(cg)->addMustPreserveSymbol(symbol);
} }
static void maybeParseOptions(lto_code_gen_t cg) { static void maybeParseOptions(lto_code_gen_t cg) {
if (!parsedOptions) { if (!parsedOptions) {
unwrap(cg)->parseCodeGenDebugOptions(); unwrap(cg)->parseCodeGenDebugOptions();
lto_add_attrs(cg); lto_add_attrs(cg);
steven_wuUnsubmitted
Not Done
ReplyInline Actions

Shouldn't this be:

if (optionParsingState ==  OptParsingState::NotParsed)
  unwrap(cg)->parseCodeGenDebugOptions();

if (optionParsingState !=  OptParsingState::Done)
    lto_add_attrs(cg);

optionParsingState = OptParsingState::Done;
steven_wu: Shouldn't this be: ``` if (optionParsingState == OptParsingState::NotParsed) unwrap(cg)…
w2yehiaAuthorUnsubmitted
Done
ReplyInline Actions

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: I considered that... the only disadvantage with guarding `unwrap(cg)->parseCodeGenDebugOptions…
steven_wuUnsubmitted
Not Done
ReplyInline Actions

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.

steven_wu: D98183 is really a debug option which is not used for production and it doesn't matter when you…
parsedOptions = true; parsedOptions = true;
} }
} }
bool lto_codegen_write_merged_modules(lto_code_gen_t cg, const char *path) { bool lto_codegen_write_merged_modules(lto_code_gen_t cg, const char *path) {
maybeParseOptions(cg); maybeParseOptions(cg);
return !unwrap(cg)->writeMergedModules(path); return !unwrap(cg)->writeMergedModules(path);
} }
Show All 23 Linesconst void *lto_codegen_compile_optimized(lto_code_gen_t cg, size_t *length) {
return CG->NativeObjectFile->getBufferStart(); return CG->NativeObjectFile->getBufferStart();
} }
bool lto_codegen_compile_to_file(lto_code_gen_t cg, const char **name) { bool lto_codegen_compile_to_file(lto_code_gen_t cg, const char **name) {
maybeParseOptions(cg); maybeParseOptions(cg);
return !unwrap(cg)->compile_to_file(name); return !unwrap(cg)->compile_to_file(name);
} }
void lto_codegen_debug_options_early(const char *opt) {
Lint: Pre-merge checks
Inline Actions

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 'opt' [readability-identifier-naming]
not useful

Lint: Pre-merge checks: clang-tidy: warning: invalid case style for function 'lto_codegen_debug_options_early'…
steven_wuUnsubmitted
Not Done
ReplyInline Actions

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.

steven_wu: If you have to pick an interface, use the new `_array` interface. This is also not correct…
w2yehiaAuthorUnsubmitted
Done
ReplyInline Actions

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: > This is also not correct since you skipped lto_add_attrs function when you called your…
if (opt) {
// Need to put each suboption in a null-terminated string before passing to
// parseCommandLineOptions().
std::vector<std::string> Options;
for (std::pair<StringRef, StringRef> o = getToken(opt); !o.first.empty();
Lint: Pre-merge checks
Inline Actions

clang-tidy: warning: invalid case style for variable 'o' [readability-identifier-naming]
not useful

Lint: Pre-merge checks: clang-tidy: warning: invalid case style for variable 'o' [readability-identifier-naming]…
o = getToken(o.second))
Options.push_back(o.first.str());
llvm::parseCommandLineOptions(Options);
parsedOptionsEarly = true;
}
}
void lto_codegen_debug_options(lto_code_gen_t cg, const char *opt) { void lto_codegen_debug_options(lto_code_gen_t cg, const char *opt) {
assert(!parsedOptionsEarly && "early option processing already happened");
SmallVector<StringRef, 4> Options; SmallVector<StringRef, 4> Options;
for (std::pair<StringRef, StringRef> o = getToken(opt); !o.first.empty(); for (std::pair<StringRef, StringRef> o = getToken(opt); !o.first.empty();
o = getToken(o.second)) o = getToken(o.second))
Options.push_back(o.first); Options.push_back(o.first);
unwrap(cg)->setCodeGenDebugOptions(Options); unwrap(cg)->setCodeGenDebugOptions(Options);
} }
▲ Show 20 Lines • Show All 210 Lines • Show Last 20 Lines