amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.58k stars 174 forks source link

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

Closed mcclure closed 1 year ago

mcclure commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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)

whitequark commented 1 year ago

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?

mcclure commented 1 year ago

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 commented 1 year ago

In my testing, this is fixed by https://github.com/YosysHQ/yosys/pull/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 commented 1 year ago

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

whitequark commented 1 year ago

@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 commented 1 year ago

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

whitequark commented 1 year ago

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

wanda-phi commented 1 year ago

It has already been merged.

whitequark commented 1 year ago

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 commented 1 year ago

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