cmd/build: support clang-style optimization flags#1762
cmd/build: support clang-style optimization flags#1762zhouguangyuan0718 wants to merge 1 commit intogoplus:mainfrom
Conversation
![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 comprehensive support for clang-style optimization levels (-O0, -O1, -O2, -O3, -Os, -Oz) throughout the build system. It adds new command-line flags, environment variable overrides via LLGO_OPTIMIZE, and ensures these levels are correctly propagated to the LLVM backend and cross-compilation toolchains. Review feedback suggests simplifying the environment variable parsing logic and centralizing the optimization level normalization code, which is currently duplicated across several packages, to improve maintainability.
internal/build/build.go
Outdated
| switch envVal { | ||
| case "0", "false", "off", "no": | ||
| return "0" | ||
| case "1", "true", "on", "yes": | ||
| return defaultOptLevel(conf) | ||
| default: | ||
| return defaultOptLevel(conf) | ||
| } |
There was a problem hiding this comment.
The default case in this switch statement has the same behavior as the case "1", "true", "on", "yes". You can simplify this by combining them into just the default case, which makes the code more concise and readable.
switch envVal {
case "0", "false", "off", "no":
return "0"
default:
return defaultOptLevel(conf)
}| func normalizeOptFlag(optLevel string, defaultLevel string) string { | ||
| optLevel = strings.TrimSpace(strings.ToLower(optLevel)) | ||
| optLevel = strings.TrimPrefix(optLevel, "-") | ||
| optLevel = strings.TrimPrefix(optLevel, "o") | ||
| switch optLevel { | ||
| case "0", "1", "2", "3", "s", "z": | ||
| return "-O" + optLevel | ||
| default: | ||
| return "-O" + defaultLevel | ||
| } | ||
| } |
There was a problem hiding this comment.
The normalization logic inside normalizeOptFlag is duplicated in internal/build/build.go and cmd/internal/flags/flags.go. To improve maintainability and avoid future inconsistencies, this logic should be centralized.
You can extract this logic into a new exported function NormalizeOptLevel in this package. normalizeOptFlag can then be simplified to use it. Subsequently, internal/build/build.go and cmd/internal/flags/flags.go can be updated to use crosscompile.NormalizeOptLevel, removing their local implementations.
// NormalizeOptLevel validates and cleans a clang-style optimization level string.
// It returns the normalized level (e.g. "2" for "-O2") and true if valid.
func NormalizeOptLevel(level string) (string, bool) {
level = strings.TrimSpace(strings.ToLower(level))
level = strings.TrimPrefix(level, "-")
level = strings.TrimPrefix(level, "o")
switch level {
case "0", "1", "2", "3", "s", "z":
return level, true
default:
return "", false
}
}
func normalizeOptFlag(optLevel string, defaultLevel string) string {
if level, ok := NormalizeOptLevel(optLevel); ok {
return "-O" + level
}
return "-O" + defaultLevel
}![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
internal/build/build.go
Outdated
| func defaultOptLevel(conf *Config) string { | ||
| if conf != nil && conf.Target != "" { | ||
| return "z" | ||
| } | ||
| return "2" | ||
| } | ||
|
|
||
| func normalizeOptLevel(level string) (string, bool) { |
There was a problem hiding this comment.
DRY violation — triplicated normalization logic. Nearly identical normalizeOptLevel/normalizeOptFlag implementations exist in three packages (cmd/internal/flags, internal/build, internal/crosscompile) with slightly different signatures. If a new level (e.g., -Og) is added, all three must be updated in lockstep.
Consider extracting a single shared implementation (e.g., in internal/build or a small internal/optlevel package) and importing it from the other two.
internal/build/build.go
Outdated
| func effectiveEnvOptLevel(conf *Config) string { | ||
| envVal := strings.TrimSpace(strings.ToLower(os.Getenv(llgoOptimize))) | ||
| if envVal == "" { | ||
| return defaultOptLevel(conf) | ||
| } | ||
| if level, ok := normalizeOptLevel(envVal); ok { | ||
| return level | ||
| } | ||
| switch envVal { | ||
| case "0", "false", "off", "no": | ||
| return "0" | ||
| case "1", "true", "on", "yes": | ||
| return defaultOptLevel(conf) | ||
| default: | ||
| return defaultOptLevel(conf) | ||
| } |
There was a problem hiding this comment.
Dead code and silent fallback. Two issues here:
-
The
case "0"andcase "1"branches (lines 1725, 1727) are unreachable —normalizeOptLevel(envVal)on line 1721 already matches those values and returns early. Only"false"/"off"/"no"and"true"/"on"/"yes"are actually reachable in the switch. -
Unrecognized values (e.g.,
LLGO_OPTIMIZE=noneorLLGO_OPTIMIZE=debug) silently fall through to the default optimization level with no warning. A user who sets an unsupported value expecting it to take effect would be surprised. Consider logging a warning for unrecognized values.
internal/build/build.go
Outdated
| export, err := crosscompile.Use(conf.Goos, conf.Goarch, conf.Target, IsWasiThreadsEnabled(), forceEspClang, effectiveOptLevel(conf)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to setup crosscompile: %w", err) | ||
| } |
There was a problem hiding this comment.
effectiveOptLevel(conf) is called 3 times in Do(). It's called here (line 230), again at line 280 for Target.OptLevel, and a third time at line 345 where it's finally stored in a local variable. Since the result is deterministic for the same conf, consider computing it once at the top of the function and reusing the local throughout.
ssa/target.go
Outdated
| func (p *Target) codegenOptLevel() llvm.CodeGenOptLevel { | ||
| switch p.OptLevel { | ||
| case "0": | ||
| return llvm.CodeGenLevelNone | ||
| case "1": | ||
| return llvm.CodeGenLevelLess | ||
| case "3": | ||
| return llvm.CodeGenLevelAggressive | ||
| default: | ||
| return llvm.CodeGenLevelDefault | ||
| } | ||
| } | ||
|
|
||
| type TargetSpec struct { |
There was a problem hiding this comment.
"2", "s", and "z" all silently map to CodeGenLevelDefault via default. While this is technically correct (LLVM's CodeGenOptLevel enum has no size-specific levels — size optimization is controlled through the pass pipeline), the implicit handling is confusing:
"2"is listed as a valid level in the comment onOptLevel(line 32) but has no explicitcase."s"and"z"advertise size optimization to users (-Os "Optimize for size") but behave identically to-O2at the codegen level.
Adding case "2", "s", "z": with a brief comment explaining the LLVM mapping would make this self-documenting.
| func normalizeOptFlag(optLevel string, defaultLevel string) string { | ||
| optLevel = strings.TrimSpace(strings.ToLower(optLevel)) | ||
| optLevel = strings.TrimPrefix(optLevel, "-") | ||
| optLevel = strings.TrimPrefix(optLevel, "o") | ||
| switch optLevel { | ||
| case "0", "1", "2", "3", "s", "z": | ||
| return "-O" + optLevel | ||
| default: | ||
| return "-O" + defaultLevel | ||
| } |
There was a problem hiding this comment.
defaultLevel parameter is not validated. The default branch constructs "-O" + defaultLevel without verifying that defaultLevel is a valid optimization level. Currently both call sites use hardcoded literals ("2" and "z"), so this is safe today. But the function signature accepts an arbitrary string — a future caller passing an unexpected value could inject unintended flags into the clang command line. Consider validating defaultLevel within the function for defense in depth.
|
Well-structured feature with good test coverage and clean flag ergonomics ( Key areas to address:
|
zhouguangyuan0718
force-pushed
the
main-opt
branch
6 times, most recently
from
6e76a78 to
086e2f6
Compare
|
/gemini review |
|
@zhouguangyuan0718 Thanks for the trigger request. This repository currently enforces member-only triggers, and your account does not have repository membership access, so xgopilot cannot run for this request. To proceed, you can:
|
There was a problem hiding this comment.
Code Review
This pull request implements a formal optimization level system across the llgo toolchain, introducing the optlevel package and integrating it into build flags and cross-compilation logic. The changes ensure consistent optimization defaults, such as -Oz for embedded targets and -O2 for host builds. The review feedback recommends centralizing optimization level validation within the public Use function to minimize code duplication and suggests improving error messages in the parser by retaining the user's original input string for better context.
| if !optLevel.IsValid() { | ||
| return export, fmt.Errorf("invalid optimization level: %s", optLevel) | ||
| } |
There was a problem hiding this comment.
To reduce code duplication, this validation of optLevel should be performed in the exported Use function, which is the entry point that calls this unexported use function. This ensures that argument validation occurs at the public API boundary. I will suggest adding the validation to the Use function in a separate comment.
| func Use(goos, goarch, targetName string, wasiThreads, forceEspClang bool, optLevel optlevel.Level) (export Export, err error) { | ||
| if targetName != "" && !strings.HasPrefix(targetName, "wasm") && !strings.HasPrefix(targetName, "wasi") { | ||
| return UseTarget(targetName) | ||
| return UseTarget(targetName, optLevel) | ||
| } | ||
| return use(goos, goarch, wasiThreads, forceEspClang) | ||
| return use(goos, goarch, wasiThreads, forceEspClang, optLevel) | ||
| } |
There was a problem hiding this comment.
As Use is an exported function, it's good practice for it to validate its arguments. Adding the optLevel validation here centralizes the validation logic, rather than duplicating it in helper functions like use.
| func Use(goos, goarch, targetName string, wasiThreads, forceEspClang bool, optLevel optlevel.Level) (export Export, err error) { | |
| if targetName != "" && !strings.HasPrefix(targetName, "wasm") && !strings.HasPrefix(targetName, "wasi") { | |
| return UseTarget(targetName) | |
| return UseTarget(targetName, optLevel) | |
| } | |
| return use(goos, goarch, wasiThreads, forceEspClang) | |
| return use(goos, goarch, wasiThreads, forceEspClang, optLevel) | |
| } | |
| func Use(goos, goarch, targetName string, wasiThreads, forceEspClang bool, optLevel optlevel.Level) (export Export, err error) { | |
| if !optLevel.IsValid() { | |
| return export, fmt.Errorf("invalid optimization level: %s", optLevel) | |
| } | |
| if targetName != "" && !strings.HasPrefix(targetName, "wasm") && !strings.HasPrefix(targetName, "wasi") { | |
| return UseTarget(targetName, optLevel) | |
| } | |
| return use(goos, goarch, wasiThreads, forceEspClang, optLevel) | |
| } |
| func Parse(level string) (Level, error) { | ||
| level = strings.TrimSpace(strings.ToLower(level)) | ||
| level = strings.TrimPrefix(level, "-") | ||
| level = strings.TrimPrefix(level, "o") | ||
| switch level { | ||
| case "0": | ||
| return O0, nil | ||
| case "1": | ||
| return O1, nil | ||
| case "2": | ||
| return O2, nil | ||
| case "3": | ||
| return O3, nil | ||
| case "s": | ||
| return Os, nil | ||
| case "z": | ||
| return Oz, nil | ||
| default: | ||
| return Unset, fmt.Errorf("invalid optimization level %q, must be one of: 0, 1, 2, 3, s, z", level) | ||
| } | ||
| } |
There was a problem hiding this comment.
The error message for an invalid optimization level currently shows the normalized level string because the level parameter is modified. It would be more user-friendly to show the original, unmodified string that the user provided. This gives them better context for what they typed incorrectly. You can achieve this by using a new variable for the normalized value and keeping the original level parameter for the error message.
func Parse(level string) (Level, error) {
normalizedLevel := strings.TrimSpace(strings.ToLower(level))
normalizedLevel = strings.TrimPrefix(normalizedLevel, "-")
normalizedLevel = strings.TrimPrefix(normalizedLevel, "o")
switch normalizedLevel {
case "0":
return O0, nil
case "1":
return O1, nil
case "2":
return O2, nil
case "3":
return O3, nil
case "s":
return Os, nil
case "z":
return Oz, nil
default:
return Unset, fmt.Errorf("invalid optimization level %q, must be one of: 0, 1, 2, 3, s, z", level)
}
}
zhouguangyuan0718
force-pushed
the
main-opt
branch
2 times, most recently
from
ba9d8d8 to
5b1c14b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1762 +/- ##
=======================================
Coverage 92.87% 92.87%
=======================================
Files 48 49 +1
Lines 13353 13411 +58
=======================================
+ Hits 12402 12456 +54
- Misses 757 761 +4
Partials 194 194 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zhouguangyuan0718
force-pushed
the
main-opt
branch
from
5b1c14b to
178ca00
Compare
Keep -O limited to clang flags and build LLDB fixtures with -O0 to preserve stack traces. Signed-off-by: ZhouGuangyuan <zhouguangyuan.xian@gmail.com>
zhouguangyuan0718
force-pushed
the
main-opt
branch
from
178ca00 to
ed88402
Compare
1 participant
Summary
This change adds clang-style optimization flags to
llgoand propagates the selected-O*level to the final clang command line.What changed
-O0/-O1/-O2/-O3/-Os/-Ozand-O=<level>optlevel.Leveltypebuild.Configcrosscompile.Use/UseTarget-O*flag intocrosscompile.Export.CCFLAGS, so the eventual clang invocation carries the requested optimization level-Olimited to clang-facing flags only; existing internal optimization behavior remains controlled by the previous build logic-O2-Ozllgo monitorhonor the same optimization flag behavior for target lookups-O0to preserve stack traces during debugger testsTesting
PATH=/opt/tools/go1.24.5.linux-amd64/go/bin:$PATH GOROOT=/opt/tools/go1.24.5.linux-amd64/go /opt/tools/go1.24.5.linux-amd64/go/bin/go test ./internal/build -run 'TestEffectiveOptLevelDefaults|TestEffectiveOptLevelOverride|TestEffectiveOptLevelIgnoresLegacyEnv|TestIsOptimizeEnabledLegacyEnv|TestTest' -count=1PATH=/opt/tools/go1.24.5.linux-amd64/go/bin:$PATH GOROOT=/opt/tools/go1.24.5.linux-amd64/go /opt/tools/go1.24.5.linux-amd64/go/bin/go test ./cmd/internal/flags ./cmd/internal/monitor ./internal/crosscompile ./ssa -count=1PATH=/opt/tools/go1.24.5.linux-amd64/go/bin:$PATH GOROOT=/opt/tools/go1.24.5.linux-amd64/go GOWORK=off <rebuilt-llgo> build -O0 -o <tmp-out> ./_lldb/lldbtest