search

YosysHQ / yosys

Yosys Open SYnthesis Suite
https://yosyshq.net/yosys/
ISC License
3.51k stars 895 forks source link

extract_fa: Fix `xor3`/`xnor3` inversion bug #4691

Open hovind opened 3 weeks ago

hovind commented 3 weeks ago

What are the reasons/motivation for this change?

The extract_fa pass seems to have issues when an odd number of inputs are inverted, this is previously reported in https://github.com/YosysHQ/yosys/issues/3879 and https://github.com/YosysHQ/yosys/issues/4573.

I think @eddiehung solved this is the xor2/xnor2 case in https://github.com/YosysHQ/yosys/pull/1297, but I the xor3/xnor3 case lacks the equivalent logic.

Explain how this is achieved.

Similarly to how it is handled in the xor2/xnor2 case, we invert the xor3/xnor3 output if the majority3 has an odd number of inverted inputs.

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

I have tried to make this break using @jix's test case from https://github.com/YosysHQ/yosys/pull/3882.

I don't have much confidence in my understanding of this code, and I hope that some discussion here will help clarify some things for me.

94215584e9bbc648c7c6184a4239d3be7df65db3 should perhaps be in a separate pull request (or probably just deleted). I left it in so I could ask if somebody could help clarify to me what the deleted code segment achieves.

84d0c8fbfc66bcd2a5ff54bd00a9cfba500ed248 I thought initially this was a bug fix, but I'm not sure invert_xy and f2i.inv_a ^ f2i.inv_b can be true at the same time?

Thanks for the amazing tool!