read_rtlil: Warn on assigns after switches in case rules

[georgerennie]

What are the reasons/motivation for this change?

Previously read_rtlil would allow assign and switch rules to be freely mixed in RTLIL case rules, but internally yosys stores the two in two separate vectors and treats assigns as if they came before switches. This change modifies the parser to reject (edit: changed to emit a warning instead) RTLIL that doesn't obey this and updates the docs. write_rtlil will only write RTLIL obeying this property.

Explain how this is achieved.

The case_body rule is split into two parts, firstly assignments and then switches. edit: A warning is run in the assign rule if the current level already has switches assigned.

If applicable, please suggest to reviewers how they can test the change.

The read_rtlil commands in the test suite should give some confidence to this.

[whitequark]

This change modifies the parser to reject RTLIL that doesn't obey this and updates the docs.

That's a breaking change to RTLIL. Are you certain it will not break other producers?

(In Amaranth, I actually had some bugs related to this, not realizing that the relative ordering of assigns and switches is disregarded. We now group assigns in a single-case switch with an empty test expression to work around that. But other producers may actually rely on the current behavior.)

[georgerennie]

Are you certain it will not break other producers?

Without knowing all the other produces I can't be sure. I suspect Amaranth is the main RTLIL producer using these rules (maybe i should look at Yosys slang too?)

not realizing that the relative ordering of assigns and switches is disregarded

I believe among assigns or among switches the ordering should be preserved, its just that they can't be mixed. Given that in #4569 it turned out sync rules were being considered in an arbitrary order based on how they hash, it wouldn't suprise me if there is something like this happening somewhere for case rules, but in theory the ordering should be preserved.

Given the unintuitive current behaviour, I would hope that other producers are not reliant on the fact that assigns are treated before switches. The hope is that this change would make it harder for users to hit this footgun where yosys implicitly reorders assigns before switches behind their back.

[povik]

Are you certain it will not break other producers?

I guess we can never be certain. Btw what other RTLIL producers do we know of? (Asking because of #4723)

I agree with making the documentation change. With the parser change I would be ok with playing it safe and parsing the malformed input too.

[povik]

maybe i should look at Yosys slang too?)

@georgerennie as a plugin it doesn’t produce textual RTLIL currently (it fills the in-memory structures)

[whitequark]

Given the unintuitive current behaviour, I would hope that other producers are not reliant on the fact that assigns are treated before switches.

What I'm worried about is a producer that knows that assigns and switches are treated separately, and emits them interspersed because it knows that. I.e. a correctly written producer.

I think emitting a warning in read_rtlil where an error would happen otherwise should catch all such cases, and in a year or two we could remove it entirely.

[georgerennie]

I think emitting a warning in read_rtlil where an error would happen otherwise should catch all such cases, and in a year or two we could remove it entirely.

I'm happy to do this too. Do we actually know of any textual RTLIL producers external to yosys other than Amaranth?

[whitequark]

I recall @nakengelhardt bringing some up in a past meeting but I don't know which one it was (it may have been some academic thing?)

As far as I know Amaranth will not be affected.