build: introduce new LTO CLI control with target-aware defaults#1694
build: introduce new LTO CLI control with target-aware defaults#1694zhouguangyuan0718 wants to merge 1 commit intogoplus:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the build system by introducing a sophisticated and flexible mechanism for managing Link-Time Optimization (LTO). It establishes intelligent, target-aware defaults for LTO, enabling it automatically for embedded and cross-compiled targets while disabling it for standard host builds. This change not only provides granular control over LTO through a new command-line flag but also integrates the LTO decision-making process earlier into the flag generation, leading to a more streamlined and optimized build experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new command-line option to control Link-Time Optimization (LTO) with target-aware defaults. The implementation of the tri-state flag and its integration into the build configuration and cross-compilation logic is well-structured. I've identified a bug in the flag parsing logic that doesn't handle the 'auto' value, and also noted that some LTO-specific linker flags are being added unconditionally, which could potentially break non-LTO builds. I have provided suggestions to address these issues. Additionally, there's a minor point about a confusing comment in a test script.
| func (o *optionalBool) SetValue(v string) error { | ||
| val, err := strconv.ParseBool(v) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid bool value %q", v) | ||
| } | ||
| o.Specified = true | ||
| o.Value = val | ||
| return nil | ||
| } | ||
|
|
||
| func (o *optionalBool) Set(v string) error { | ||
| return o.SetValue(v) | ||
| } |
There was a problem hiding this comment.
The Set method for optionalBool doesn't handle the value "auto". According to the flag's usage string, "auto" is a valid option. If a user provides -lto=auto, strconv.ParseBool("auto") will fail, causing an error. The Set method should be updated to handle "auto" gracefully, likely by resetting the optionalBool to its "unspecified" state. Additionally, the SetValue helper function can be removed for clarity by incorporating its logic directly into Set.
| func (o *optionalBool) SetValue(v string) error { | |
| val, err := strconv.ParseBool(v) | |
| if err != nil { | |
| return fmt.Errorf("invalid bool value %q", v) | |
| } | |
| o.Specified = true | |
| o.Value = val | |
| return nil | |
| } | |
| func (o *optionalBool) Set(v string) error { | |
| return o.SetValue(v) | |
| } | |
| func (o *optionalBool) Set(v string) error { | |
| if v == "auto" { | |
| o.Specified = false | |
| o.Value = false | |
| return nil | |
| } | |
| val, err := strconv.ParseBool(v) | |
| if err != nil { | |
| return fmt.Errorf("invalid value %q for lto flag; must be true, false, or auto", v) | |
| } | |
| o.Specified = true | |
| o.Value = val | |
| return nil | |
| } |
| ccflags = append(ccflags, "-march=rv64gc") | ||
| // codegen option should be added to ldflags for lto | ||
| ldflags = append(ldflags, "-mllvm", "-march=rv64gc") |
There was a problem hiding this comment.
The linker flag -mllvm -march=rv64gc is added unconditionally for the riscv64 architecture. The comment indicates this is for LTO. Passing LLVM-specific options to the linker might cause issues for non-LTO builds. This flag should only be added when enableLTO is true.
| ccflags = append(ccflags, "-march=rv64gc") | |
| // codegen option should be added to ldflags for lto | |
| ldflags = append(ldflags, "-mllvm", "-march=rv64gc") | |
| ccflags = append(ccflags, "-march=rv64gc") | |
| if enableLTO { | |
| // codegen option should be added to ldflags for lto | |
| ldflags = append(ldflags, "-mllvm", "-march=rv64gc") | |
| } |
| if config.CodeModel != "" { | ||
| ccflags = append(ccflags, "-mcmodel="+config.CodeModel) | ||
| // codegen option should be added to ldflags for lto | ||
| ldflags = append(ldflags, "-mllvm", "-code-model="+config.CodeModel) | ||
| } | ||
| if config.TargetABI != "" { | ||
| ccflags = append(ccflags, "-mabi="+config.TargetABI) | ||
| // codegen option should be added to ldflags for lto | ||
| ldflags = append(ldflags, "-mllvm", "-target-abi="+config.TargetABI) | ||
| } |
There was a problem hiding this comment.
The linker flags for code-model and target-abi are added unconditionally. As the comments suggest, these are needed for LTO. To avoid potential issues in non-LTO builds, these flags should be added to ldflags only when enableLTO is true.
if config.CodeModel != "" {
ccflags = append(ccflags, "-mcmodel="+config.CodeModel)
if enableLTO {
// codegen option should be added to ldflags for lto
ldflags = append(ldflags, "-mllvm", "-code-model="+config.CodeModel)
}
}
if config.TargetABI != "" {
ccflags = append(ccflags, "-mabi="+config.TargetABI)
if enableLTO {
// codegen option should be added to ldflags for lto
ldflags = append(ldflags, "-mllvm", "-target-abi="+config.TargetABI)
}
}
_demo/embed/targetsbuild/build.sh
Outdated
| "riscv32" | ||
| "riscv64" | ||
| "rp2040" | ||
| "nintendoswitch" # undefined symbol under lto, should not work when no-lto |
There was a problem hiding this comment.
The comment here is a bit confusing. "undefined symbol under lto" suggests it fails with LTO enabled, which makes sense for adding it to the ignore list given LTO is now on by default for targets. However, "should not work when no-lto" seems to suggest it's also expected to fail without LTO. Could you clarify this comment for future reference? Perhaps something like "Fails with LTO due to undefined symbols, and has separate known issues without LTO." would be clearer.
|
Good approach overall — the tri-state flag design with target-aware defaults is clean, and the |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| // Enable ThinLTO, using default lto kind(thinlto). | ||
| export.LDFLAGS = append(export.LDFLAGS, "-Wl,--lto-O0") | ||
| } | ||
| if clangRoot != "" { |
There was a problem hiding this comment.
--lto-O0 tells the linker to skip all optimization passes during the LTO step. This pays the full cost of ThinLTO (bitcode serialization, cross-module summary analysis, importing) while delivering zero optimization benefit — no cross-module inlining, dead global elimination, or interprocedural constant propagation.
Code compiled with -flto=thin may defer certain optimization decisions to link time; with O0 at link time those deferred optimizations never happen, potentially producing worse output than a non-LTO build.
Consider --lto-O2 (or --lto-O1 for a link-time vs optimization tradeoff). The same applies to line 510 in UseTarget().
| ccflags = append(ccflags, "-fforce-enable-int128") | ||
| case "riscv64": | ||
| ccflags = append(ccflags, "-march=rv64gc") | ||
| // codegen option should be added to ldflags for lto | ||
| ldflags = append(ldflags, "-mllvm", "-march=rv64gc") |
There was a problem hiding this comment.
The comments say "codegen option should be added to ldflags for lto", but these -mllvm flags are appended unconditionally regardless of whether enableLTO is true. The same pattern appears for CodeModel (line 593) and TargetABI (line 598).
-mllvm flags are only meaningful when the linker runs the LLVM backend during LTO codegen. When LTO is off, the linker doesn't invoke codegen, so these flags are inert at best or may trigger warnings depending on the LLD version. These blocks should be gated on enableLTO, matching the guard on lines 508–513.
| export.CCFLAGS = append(export.CCFLAGS, "-flto=thin") | ||
| } | ||
|
|
There was a problem hiding this comment.
In this non-target use() path, -flto=thin is added to CCFLAGS but not to CFLAGS. In the UseTarget() path (lines 511–512), it's correctly added to both cflags and ccflags. If any compilation steps use only CFLAGS, those object files won't contain LLVM bitcode and will pass through the LTO link step as opaque machine code, creating a partially-LTO build. Consider adding -flto=thin to export.CFLAGS as well for consistency.
_demo/embed/targetsbuild/build.sh
Outdated
| "riscv32" | ||
| "riscv64" | ||
| "rp2040" | ||
| "nintendoswitch" # undefined symbol under lto, should not work when no-lto |
There was a problem hiding this comment.
This comment is contradictory: "undefined symbol under lto, should not work when no-lto." The first clause says it fails with LTO; the second says it should not work without LTO. Based on context (LTO is now on by default for target builds), the likely intent is:
nintendoswitch: has undefined symbol errors under LTO; added to ignore list since LTO is now enabled by default for target builds
| var serialPort []string | ||
| if flags.Target != "" { | ||
| conf, err := crosscompile.UseTarget(flags.Target) | ||
| conf, err := crosscompile.UseTarget(flags.Target, flags.ResolveLTO(true)) |
There was a problem hiding this comment.
This hardcodes true as the default, bypassing the centralized Config.ltoEnabled() logic. The build path uses Config.ltoEnabled() which derives the default from c.Target != "". Today the results are equivalent (monitor only calls UseTarget when flags.Target != ""), but if the resolution logic changes later, this code path would need a separate manual update. Consider routing through Config.ltoEnabled() for consistency, or at minimum adding a comment explaining why the hardcoded default is correct here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1694 +/- ##
==========================================
+ Coverage 93.15% 93.19% +0.03%
==========================================
Files 48 48
Lines 13331 13343 +12
==========================================
+ Hits 12419 12435 +16
+ Misses 725 722 -3
+ Partials 187 186 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zhouguangyuan0718
force-pushed
the
main_lto_embed
branch
2 times, most recently
from
af5b84e to
6e34d25
Compare
This change newly introduces end-to-end LTO control in llgo. - add a new tri-state -lto option (auto/true/false) - default LTO ON for embedded target builds and OFF for non-target builds - allow explicit user override in both paths - apply LTO decision at crosscompile flag generation time - make monitor follow the same LTO rule path - add/update tests for default and override behavior Signed-off-by: ZhouGuangyuan <zhouguangyuan.xian@gmail.com>
zhouguangyuan0718
force-pushed
the
main_lto_embed
branch
from
6e34d25 to
9945d01
Compare
1 participant
Summary
-ltooption (auto/true/false)-targetbuilds and OFF for non-target buildsllgo monitorfollow the same LTO rule pathDetails
cmd/internal/flagsinternal/crosscompile-flto=thinand--lto-*emission on the resolved LTO switchValidation
go test ./internal/crosscompile -run TestUseLTOFlagsControlledByOptiongo test ./cmd/internal/monitor ./cmd/internal/flags ./internal/crosscompile -run TestUseLTOFlagsControlledByOptiongo test ./cmd/internal/flags ./cmd/internal/build ./cmd/internal/run ./cmd/internal/test ./cmd/internal/install ./cmd/internal/monitor ./internal/build -run TestLTOEnabled