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

write_verilog: don't emit code with dangling else related to wrong condition #4150

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Jan 23, 2024

It is embarrassing to have written it, but also, three independent reviewers missed it too :D

See amaranth-lang/amaranth#1049.

Issue introduced in #4090

@whitequark
Copy link
Member Author

I can't reproduce the test failure locally for some reason. That's odd.

@povik
Copy link
Member

povik commented Jan 23, 2024

Oh no

@povik
Copy link
Member

povik commented Jan 23, 2024

In any case I feel like we need some tests here.

@povik
Copy link
Member

povik commented Jan 23, 2024

I can't reproduce the test failure locally for some reason. That's odd.

That looks like the issue we saw yesterday due to iverilog updating, but since your PR is based on #4148 the CI here should have been pinned to a good iverilog revision already. @mmicko ideas?

@povik
Copy link
Member

povik commented Jan 24, 2024

@whitequark OK with you if I push to your branch? I would rebase (to fix CI), and maybe add some test.

@whitequark
Copy link
Member Author

Go ahead please.

@povik
Copy link
Member

povik commented Jan 24, 2024

Rebased

@@ -1996,36 +1982,52 @@ bool dump_proc_switch_ifelse(std::ostream &f, std::string indent, RTLIL::SwitchR
}
}

dump_attributes(f, indent, sw->attributes);
Copy link
Member

Choose a reason for hiding this comment

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

So this is something we were missing before, unrelated to the fix

Copy link
Member Author

Choose a reason for hiding this comment

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

We were dumping slightly different attributes (for case rules and not switch itself), the switch is probably better.

Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

Looks fine

@whitequark
Copy link
Member Author

Thanks @povik!

@whitequark whitequark merged commit 08fd47e into YosysHQ:master Jan 24, 2024
17 checks passed
@whitequark whitequark deleted the dangling-else branch January 24, 2024 16:32
@whitequark whitequark restored the dangling-else branch January 24, 2024 16:32
@whitequark whitequark deleted the dangling-else branch January 24, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants