fix: separate EQUIVALENCE_CHECK from RUN_EQY to avoid requiring eqy#4108
fix: separate EQUIVALENCE_CHECK from RUN_EQY to avoid requiring eqy#4108oharboe wants to merge 4 commits into
Conversation
EQUIVALENCE_CHECK now only controls writing equivalence check files. The new RUN_EQY variable (default 0) gates actually executing the eqy tool, so builds don't fail when eqy is not installed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@maliberty Interesting... Is master broken? Does eqy have side-effects??? |
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Auto-detect the eqy tool via `command -v` and set RUN_EQY ?= 1 so equivalence checking runs automatically when the tool is installed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
| repair_timing_helper | ||
|
|
||
| if { $::env(EQUIVALENCE_CHECK) } { | ||
| if { $::env(EQUIVALENCE_CHECK) && $::env(RUN_EQY) } { |
There was a problem hiding this comment.
I don't follow why we need a second variable to control this?
There was a problem hiding this comment.
See documentation:
- Should we do an EQY check at all? Do we have the tool?
- Should we do an EQY check on this module?
This improves the user experience and helps bazel because the eqy isn't part of normal flow and I don't currently install that tool. We can later, but I think that whether to run eqy and whether to run it on this module should be two concerns.
There was a problem hiding this comment.
I don't see how those are separate. What does it mean to "run eqy" but not on this module?
There was a problem hiding this comment.
If you don't have eqy installed, then CTS fails today on modules which have EQUIVALENCE_CHECK =1 (or ?=1).
With this change, if you don't have eqy installed, then RUN_EQY defaults to 0, and cts no longer fails.
There was a problem hiding this comment.
Designs with EQUIVALENCE_CHECK=1 (e.g. swerv_wrapper) call exec eqy during CTS.
If eqy isn't installed on the machine, CTS crashes. An optional tool
shouldn't break the build.
The fix: one variable becomes two
| Variable | What it does | Default |
|---|---|---|
EQUIVALENCE_CHECK |
Write before/after verilog files (cheap, no tool needed) | 0 (unchanged) |
RUN_EQY |
Actually execute the eqy binary |
1 if eqy is in $PATH, else 0 |
If you have eqy installed: nothing changes. Auto-detected, auto-enabled.
If you don't have eqy: builds stop crashing.
The 4 files that matter
The core logic change — cts.tcl line 71:
if { $::env(EQUIVALENCE_CHECK) && $::env(RUN_EQY) } {
run_equivalence_test
}Previously this only checked EQUIVALENCE_CHECK. Now it requires both.
The verilog write at line 62 still only needs EQUIVALENCE_CHECK (no tool required).
Auto-detection — variables.mk lines 135-137:
ifneq ($(shell command -v eqy),)
export RUN_EQY ?= 1
endifSame pattern already used for stdbuf a few lines above.
Variable definition — variables.yaml RUN_EQY block
Docs — FlowVariables.md RUN_EQY entry
"Why two variables?"
The question from review: "What does it mean to run eqy but not on this module?"
It's the reverse that matters: module wants equivalence checking (EQUIVALENCE_CHECK=1)
but machine doesn't have eqy installed. Without the split, that = build failure.
With the split, the verilog files still get written (useful for running eqy later
or elsewhere), the build just skips the tool it can't run.
To merge
Per your review comment: merge master and drop the OpenROAD submodule bump.
The actual change is just the variable split across the 4 files above.
|
Failures on master were resolved over the weekend, except the on-going yosys non-determinism. Merge master and drop the OR update. |
|
I think this is more easily solved by just removing eqy entirely. We have KF with LEC_CHECK so let's retire EQUIVALENCE_CHECK. |
Begone #4117 ! |
EQUIVALENCE_CHECK now only controls writing equivalence check files. The new RUN_EQY variable (default 0) gates actually executing the eqy tool, so builds don't fail when eqy is not installed.