Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amaranth emitted invalid Verilog (Amaranth conditionals containing only comb assignments results in Verilog "empty case" error) #931

Closed
mcclure opened this issue Oct 7, 2023 · 12 comments · Fixed by #955
Assignees
Milestone

Comments

@mcclure
Copy link
Contributor

mcclure commented Oct 7, 2023

Using Amaranth (0.4.dev198+g05cb82b) to target Cyclone V (Analogue Pocket), which means generating Verilog which gets parsed by Quartus. Got an error of invalid Verilog from Quartus.

Errors contained phrases such as

near text: "endcase"; expecting an operand
near text: "end"; expecting "endcase"
near text: "always"; expecting "endcase"

Repro code:

https://github.com/mcclure/analogue-core-template-amaranth/tree/control-test COMMIT 2354a5e

Build steps (contains system local paths):

(cd src/fpga/amaranth_core/ && python.exe -m pdm generate) && \
(cd src/fpga && /mnt/d/intelFPGA_lite/22.1std/quartus/bin64/quartus_sh.exe --flow compile ap_core)

Error messages:

quartus_errors.txt

Referenced amaranth_core.v:

amaranth_core.v.txt

This code is a slight modification of code which worked.

I have not yet tried clearing my pdm.lock.

@mcclure
Copy link
Contributor Author

mcclure commented Oct 7, 2023

Cleared pdm.lock, upgraded to Amaranth 0.4.dev214+gc7da6c1. No apparent change. Can re-upload errors/core.v from new code if helpful.

@mcclure
Copy link
Contributor Author

mcclure commented Oct 7, 2023

Looking at the error site:

In my code, I have this complex tree of checking conditions such as "bottom_left_diag" "top_left_diag"
Based on this I set various values in the "comb" domain
The way it has been described to me amaranth comb signals "do not exist"
That is, it ought to be no actual verilog will be emitted at the point where they are set. Instead they are inlined into some other expression later
Amaranth is emitting the tree of IFs but putting nothing inside them (possibly because nothing is inside them but combs, which do not exist)

@mcclure mcclure changed the title Something I did made Amaranth emit invalid Verilog Amaranth emitted invalid Verilog (Amaranth conditionals containing only comb assignments results in Verilog "empty case" error) Oct 7, 2023
@whitequark
Copy link
Member

The way it has been described to me amaranth comb signals "do not exist"
That is, it ought to be no actual verilog will be emitted at the point where they are set. Instead they are inlined into some other expression later

I... don't think that's accurate at all?

mwkmwkmwk pushed a commit to YosysHQ/yosys that referenced this issue Oct 7, 2023
The Verilog grammar does not allow an empty case.  Most synthesis tools
are quite permissive about this, but Quartus is not.  This causes
problems for amaranth with Quartus (see amaranth-lang/amaranth#931).
@mcclure
Copy link
Contributor Author

mcclure commented Oct 8, 2023

I... don't think that's accurate at all?

Okay. Well in any case these case statements were empty as if their contents had been optimized away.

@mcclure
Copy link
Contributor Author

mcclure commented Oct 8, 2023

In my testing, this is fixed by YosysHQ/yosys#3992

(With that patch, my code will successfully build. When run, the code paints a black screen. Because this is the first time I have compiled it, it is as of yet unclear to me whether that is correct.)

@mcclure
Copy link
Contributor Author

mcclure commented Oct 8, 2023

Update: Fixed some logic bugs so the screen is no longer back and can confirm, yosys#3992 fixes the issue

@whitequark
Copy link
Member

@wanda-phi Is this something that was broken in Yosys since forever, or is this relatively new? Asking because we might want to update the Yosys version constraint in response to this issue.

@wanda-phi
Copy link
Member

I'm reasonably sure that has been broken since forever.

@whitequark
Copy link
Member

Thanks. In that case once the upstream PR gets merged I'll need to bump amaranth-yosys dependency and also required Yosys version.

@whitequark whitequark self-assigned this Oct 8, 2023
@wanda-phi
Copy link
Member

It has already been merged.

@whitequark
Copy link
Member

Ah right. In that case I'm waiting until the next version bump so that I can do something reasonably consistent in face of this monstrosity.

@whitequark
Copy link
Member

Yosys 0.35 was released, so that can become our new Yosys requirement.

gussmith23 pushed a commit to uwsampl/yosys that referenced this issue Nov 20, 2023
The Verilog grammar does not allow an empty case.  Most synthesis tools
are quite permissive about this, but Quartus is not.  This causes
problems for amaranth with Quartus (see amaranth-lang/amaranth#931).
whitequark added a commit to whitequark/amaranth that referenced this issue Nov 21, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants