jayfoad wrote:
Passing on some LLM-aided code review comments in case they're helpful. This is
not my area of expertise.
---
1. **Blocking: legacy LTO.** `llvm-lto` and libLTO (`LTOCodeGenerator`,
`ThinLTOCodeGenerator`, `lto_codegen_debug_options`,
`thinlto_debug_options`) need to populate the contexts they create before
this lands. Something like:
- `LTOCodeGenerator`: parse the debug options into its own
`LibraryOptionsContext` and `assign()` it into `Context`, or set
`Config.LibraryOptions`.
- `ThinLTOCodeGenerator`: hold a `LibraryOptionsContext` and `assign()`
it into each per-module `LLVMContext` it creates
(`ThinLTOCodeGenerator.cpp:1005`, `:1177`).
- `llvm-lto`: `cl::parseCommandLineOptions(Context.getLibraryOptions(), …)`
the way #233162, #233182 and #233194 did for other tools, plus forwarding
to the ThinLTO generator.
That could be a separate prerequisite PR (`[llvm-lto] Parse library
options into LLVMContext`). The libLTO part deserves a test, since it is
ld64's `-mllvm` path and only the two `llvm-lto` tests currently catch it.
2. **`PipelineTuningOptions` should follow the same rule.** As it stands the
PR swaps one silent `Global` fallback (PassBuilder) for another (default
PTO). The PTO-derived flags (`-enable-merge-functions`,
`-enable-loopinterchange`, `-eagerly-invalidate-analyses`,
`-enable-devirtualize-speculatively`, `-forget-scev-loop-unroll`,
`-licm-mssa-*`) are silently ignored by the callers listed above. Options:
- give `PipelineTuningOptions` a `const LLVMContext &` or
`const LibraryOptionsContext &` constructor and delete the `nullptr`
default;
- have `PassBuilder` initialise those PTO fields from `Context` when the
PTO was default-constructed;
- at minimum, convert the in-tree default-PTO callers (llc NewPMDriver,
llvm-reduce RunIRPasses, ThinLTOCodeGenerator, the C API bindings,
offload JIT, polly) to `PipelineTuningOptions(&Ctx.getLibraryOptions())`.
Otherwise the description's claim that callers no longer silently get the
global values is only half true.
3. **PTO/Context consistency.** Before the PR, `PTO.LibraryOptions` fed both
the PTO defaults and the PassBuilder options, so they always agreed. Now
the PTO can be built from context A and the PassBuilder from context B.
Consider asserting, or at least documenting, that they should be the same
context.
4. **API churn for downstream users.** Making `LLVMContext &` a required
first parameter breaks every out-of-tree `PassBuilder` user (Rust, Julia,
Swift, plugins, …), and it reorders positional arguments. That's within
LLVM's API policy, but a release-note entry under "Changes to the C++ API"
would help. Consider keeping a deprecated `PassBuilder(TargetMachine*, …)`
overload for one release if it's still needed.
5. **Lifetime.** `PassBuilder` now holds `LLVMContext &`. The new doc comment
says the context must outlive the PassBuilder. All in-tree uses I checked
satisfy that. `offload/.../JIT.cpp` creates a local `LLVMContext` just to
carry options. That's fine, but a reader might find it surprising; a
short comment there would help.
6. **(checked, OK) llvm-opt-fuzzer.** The `static cl::LibraryOptionsContext
LibraryOptions;` moved from a function-local static to namespace scope,
and the comment "Created after the targets register their library
options" was dropped. That's fine:
- the constructor is trivial;
- what matters (cf. 793ff02cdb35) is that the targets are initialised
before *parsing*, and `InitializeAllTargets()` still precedes
`parseFuzzerCLOpts`.
The per-input `assign()` into each fresh `LLVMContext` is needed and
correct.
https://github.com/llvm/llvm-project/pull/233150
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
