YosysHQ / yosys

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

Tweaked the representation of nets and buffers in the Show command graph. #4247

Closed zapta closed 7 months ago

zapta commented 7 months ago

Two tweaks to the generated graph.

  1. Nets changed from a diamond shapes to plain text with the net name. This provides the visual load of the nets while preserving their names.

  2. Buffers changed from a period shape to a plain text with the label "[BUF]", providing hint what they are while keeping the visual load low

Before before

After after

Dot files Archive.zip

whitequark commented 7 months ago

Personally, I find it unconvincing that the new representation is any better (or any worse) than the old one. Both have issues in slightly different ways.

povik commented 7 months ago

2. Buffers changed from a period shape to a plain text with the label "[BUF]", providing hint what they are while keeping the visual load low

This makes buffers more visible, why? I suspect this is because it looks better when used with -colors, but it makes the output more cluttered. Just recently we removed an explicit BUF label, and this would change it back.

zapta commented 7 months ago

@povik, the rationale behind the "[BUF]" is to provide to readers an indication what that node mean. Below are the three version of the buffer. The "[BUF]" one provide a balance between providing the information while keep low clutter.

buf1

buf2

buf3

whitequark commented 7 months ago

is to provide to readers an indication what that node mean

Yosys developers already know what the node means though.

zapta commented 7 months ago

@whitequark, yes, it targets new readers, eliminating the question "what does this dot mean?", without much burden on readers that already know.

whitequark commented 7 months ago

It was enough burden that the representation was changed.

zapta commented 7 months ago

Are you referring to this one with the BUF blocks? I didn't go back to that burden level.

[image: image.png]

On Thu, Feb 29, 2024 at 8:00 AM Catherine @.***> wrote:

It was enough burden that the representation was changed.

— Reply to this email directly, view it on GitHub https://github.com/YosysHQ/yosys/pull/4247#issuecomment-1971448979, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQVMQL3T2EOM5KO5FKYOF3YV5IBFAVCNFSM6AAAAABD7Q37QSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRGQ2DQOJXHE . You are receiving this because you authored the thread.Message ID: @.***>

zapta commented 7 months ago

This yosys application suggests that the diagram is also targeting users and learners. I hope that is still the case.

image
nakengelhardt commented 7 months ago

The reason I changed the "BUF" nodes to a dot was that there were a lot of issues filed by confused users that asked how to make yosys remove the buffers (that didn't really exist in the netlist, but were just a visual representation for two connected wires) and I got tired of answering them. I haven't seen any new questions like that filed since the change, so I feel quite confident in saying that the dot shape is less confusing to users.

zapta commented 7 months ago

Thanks @nakengelhardt. I will restore the 'point' shape and will submit a new clean PR.

nakengelhardt commented 7 months ago

For the plaintext I guess it can be an option, I personally dislike the wide space between the place where the arrow points and the actual text, especially for short wire names, but I can see that it's a taste question. The default should remain the diamond but I would be ok with a generic option to override it, something like show -wireshape foo.

zapta commented 7 months ago

Sounds good. Thanks.

On Thu, Feb 29, 2024 at 10:14 AM N. Engelhardt @.***> wrote:

For the plaintext I guess it can be an option, I personally dislike the wide space between the place where the arrow points and the actual text, especially for short wire names, but I can see that it's a taste question. The default should remain the diamond but I would be ok with a generic option to override it, something like show -wireshape foo.

— Reply to this email directly, view it on GitHub https://github.com/YosysHQ/yosys/pull/4247#issuecomment-1971693738, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQVMQOV6BREUCJBKNL2TWDYV5XZVAVCNFSM6AAAAABD7Q37QSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRGY4TGNZTHA . You are receiving this because you modified the open/close state.Message ID: @.***>

zapta commented 7 months ago

Hi @nakengelhardt, I have the -wireshape code working, but before I send a PR, I would like to ask you one more question.

You mentioned that you dislike the space in the plaintext nodes and I found a way to eliminate it (see below). Would you consider having this is the new default (and only) behavior, resulting with simpler code, or should I go ahead with the --wireshape PR?

Original/default: _default

With -wireshape=plaintext: _plaintext

BTW, https://edotor.net/ is a great site to experiment with .dot files.

zapta commented 7 months ago

Opening for the ongoing discussion.