Skip to content
This repository has been archived by the owner on Apr 20, 2024. It is now read-only.

Correctly specify width of default zero output value #74

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

albert-magyar
Copy link
Contributor

I believe this should fix the bug posted by Young Kim to the mailing list yesterday (2/10). The mux expression must have matching widths for its arguments. I think FIRRTL should be giving better error messages, but this should be the correct fix.

The error message is:
…/chipyard/sims/verilator/generated-src/freechips.rocketchip.system.DefaultTestConfig/freechips.rocketchip.system.DefaultTestConfig.harness.mems.v:842: Operator COND expects 64 bits on the Conditional False, but Conditional False's CONST '1'h0' generates 1 bits.
... Use "/* verilator lint_off WIDTH */" and lint_on around source to disable this message.
%Error: Exiting due to 1 warning(s)

Copy link
Member

@edwardcwang edwardcwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test?

@albert-magyar albert-magyar merged commit 8ca8765 into master Feb 12, 2020
@albert-magyar
Copy link
Contributor Author

@abejgonzalez I believe CI is not working. I don't think there are any isolated tests of the macro compiler.

@albert-magyar albert-magyar deleted the correct-width-mux branch February 12, 2020 03:04
@edwardcwang
Copy link
Member

@albert-magyar
Copy link
Contributor Author

Also, this is not actually a FIRRTL bug but a mismatch in Verilog emission with Verilator expectations.

@albert-magyar
Copy link
Contributor Author

albert-magyar commented Feb 12, 2020

The "correct" way to handle this would be to run PadWidths (to make the width-matching of the two mux subexpressions explicit) and then either Legalize or ConstantPropagation, each of which will convert a pad on a literal to a wider literal.

There was an old comment in MacroCompilerTransforms about a TODO to support actually running LowFirrtlOptimization (which includes those passes), but I did not do that, as that would be far more invasive.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants