Repository navigation
[codegen] Error instead of silently overriding an explicit codegen version for proc-scoped channels - #5163
Open
12yuuuu wants to merge 1 commit into
Open
[codegen] Error instead of silently overriding an explicit codegen version for proc-scoped channels#516312yuuuu wants to merge 1 commit into
12yuuuu wants to merge 1 commit into
Conversation
…rsion for proc-scoped channels Since google#3679, a package with proc-scoped channels forced codegen 1.5 even when the user explicitly asked for another version (`codegen_version = "1.0"` on a BUILD target, or `--codegen_version=1.0`). The user's choice was replaced without any message, so they could believe they were exercising codegen 1.0 while actually running 1.5. The codegen version is now resolved in one place, `ResolveCodegenVersion()` in xls/tools/codegen.cc: * no version requested (0 / default) -> 1.5, as before; * 1.5 requested -> 1.5; * 1.0 or 2.0 requested + proc-scoped IR -> InvalidArgumentError that names the requested version and the two ways forward (use 1.5, or convert the DSLX with --lower_to_proc_scoped_channels=false). 1.5 is treated as the required version for proc-scoped channels per google#3679. Packages without proc-scoped channels are unaffected. The version is resolved exactly once, in CodegenMetadata::Create(), which every entry point of xls/tools/codegen.h goes through (Schedule, Codegen, ScheduleAndCodegen, ScheduleAndConvertToBlock); codegen_main therefore fails before scheduling. Note that this means Schedule() alone (scheduling with codegen flags but no RTL generation) now also rejects an incompatible explicit version. The resolved version is stored in the CodegenOptions, and the three former copies of the override (codegen_main.cc, CodegenFromMetadata, ScheduleAndConvertToBlock) are removed; the two dispatch sites only check that the version has been resolved. xls/examples/codegen_options: the two f_proc targets asked for codegen_version "2" but, because of the override, were generated with 1.5 all along; they now say "1.5", so their output does not change. Tests (codegen_main_test.py, new proc-scoped IR input): default version works, explicit 1.5 works and matches the default, explicit 1 / 1.0 / 2 / 2.0 are rejected with a message naming the version and "proc-scoped channels", and explicit 1.0 is still accepted for a proc with global channels. Fixes google#4554 Assisted-by: Claude (Anthropic) — code and tests drafted with AI assistance; reviewed and tested by the author
allight
approved these changes
Oct 19, 2026
Contributor
|
Thanks for the contribution. LGTM |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4554
Since #3679, when a package uses proc-scoped channels, codegen always switches to version 1.5, even if the user explicitly set
--codegen_version=1.0(or2). There is no error or warning. This override is incodegen_main.cc, and also in two more places inxls/tools/codegen.cc.What this PR changes
xls/tools/codegen.cc: addResolveCodegenVersion()and call it once inCodegenMetadata::Create(). The three old overrides are removed. Rules: unset or0-> 1.5 (same as before);1.5-> 1.5; explicit1.0or2.0with proc-scoped channels ->InvalidArgumentError. The error message says which version was requested and gives two options (use 1.5, or convert with--lower_to_proc_scoped_channels=false). Packages without proc-scoped channels are not affected.xls/examples/codegen_options/BUILD: the twof_proctargets were set to"2", but because of the override they were actually generated by 1.5 all along. Changed them to"1.5", so the output is the same.codegen_main_test.py.Two design choices. I think both are reasonable, but please tell me if you want them changed.
ResolveCodegenVersion().CodegenMetadata::Create(), so callingSchedule()alone with this combination also fails. I think this is correct:Schedule()already parses and validates all codegen flags (for example, a bad version string already fails there), so a version that cannot be used for this package is just another invalid flag. This is also what letscodegen_mainfail before scheduling, which can take a long time. The other caller,io_printer_main, gets the same error for the same wrong flags. If you preferSchedule()to not check this, it is about 10 lines to limit the check to the codegen paths.Testing (macOS, Bazel 8.7.0, on top of 54b1b49):
//xls/tools:codegen_main_testand//xls/examples/codegen_options:verilog_targets_buildpass. On unpatched main, the same proc-scoped IR with--codegen_version=1.0succeeds silently, and its output is byte-identical to the 1.5 output. I did not run the full test suite locally, so I rely on CI for that.Assisted-by: Claude (Anthropic). Code and tests were drafted with AI assistance; I reviewed the diff and ran the tests above myself.