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

make: gui_ prerendering #2610

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

oharboe
Copy link
Collaborator

@oharboe oharboe commented Dec 3, 2024

less click and wait when the GUI opens

Clock tree prerendering is still missing, because there is no way yet to disable the clock tree rendering from .tcl after the clock tree has been rendered.

Some progress added so that the user can tell what it is that is taking time. Also added a final gui::unminimize log item so the user knows when the GUI should have unminimized(which it doesn't on Ubuntu 24.04 with Wayland yet, known issue filed).

make gui_route
[deleted]
estimate_parasitics -global_routing
Populating timing paths...OK
Prerendering Placement heatmap...
Prerendering Routing heatmap...
Prerendering RUDY heatmap...
Prerendering Power heatmap...
gui::select_chart "Endpoint Slack"
gui::update_timing_report
gui::unminimize

gui::select_clockviewer_clock $clock_name
save_image -resolution $resolution $::env(REPORTS_DIR)/cts_${clock_name}_layout.webp
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maliberty How do I unselect the clock tree and return the GUI to the default view?

make gui_cts

image

@maliberty
Copy link
Member

@gadfort "How do I unselect the clock tree and return the GUI to the default view?"

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 4, 2024

@maliberty Should grt::have_routes return false silently if the instances are not placed?

>>> grt::have_routes
[ERROR GRT-0010] Instance ctrl.state.out\[0\]$_DFF_P_ is not placed.
[ERROR GUI-0070] GRT-0010

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 4, 2024

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 4, 2024

@maliberty Tried on a larger design, works nicely. Nits need to be fixed, broken out as issues.

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 5, 2024

@gadfort "How do I unselect the clock tree and return the GUI to the default view?"

How about The-OpenROAD-Project/OpenROAD#6307 ?

@oharboe oharboe force-pushed the make-gui-less-click-and-wait branch from d918d86 to 0e63d2b Compare December 6, 2024 06:47
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
less click and wait when the GUI opens for large designs

clock tree prerendering is still missing, because there is
no way yet to disable the clock tree rendering from .tcl after
the clock tree has been rendered.

Some progress added so that the user can tell what it is
that is taking time. Also added a final gui::unminimize
log item so the user knows when the GUI should have
unminimized(which it doesn't on Ubuntu 24.04 with Wayland
yet, known issue filed).

make gui_route
[deleted]
estimate_parasitics -global_routing
Populating timing paths...OK
Prerendering Placement heatmap...
Prerendering Routing heatmap...
Prerendering RUDY heatmap...
Prerendering Power heatmap...
gui::select_chart "Endpoint Slack"
gui::update_timing_report
gui::unminimize

The-OpenROAD-Project/OpenROAD#6074

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@oharboe oharboe force-pushed the make-gui-less-click-and-wait branch from 0e63d2b to 64d013d Compare December 6, 2024 06:50
@oharboe oharboe changed the title DO NOT REVIEW make: gui_ prerendering make: gui_ prerendering Dec 6, 2024
@oharboe oharboe requested a review from maliberty December 6, 2024 06:52
@oharboe
Copy link
Collaborator Author

oharboe commented Dec 6, 2024

@maliberty Ready for review. Preprendering clock tree is still missing, but that's a simple change once this is through and that missing clock tree feature is available in OpenROAD.

@oharboe oharboe marked this pull request as ready for review December 6, 2024 06:53
Comment on lines 70 to 80
# FIXME reenable when there is a way to disable the rendered clock tree
#
# foreach clock [get_clocks *] {
# if { [llength [get_property $clock sources]] > 0 } {
# set clock_name [get_name $clock]
# save_clocktree_image -clock $clock_name \
# -width 100 -height 100 \
# $::env(OBJECTS_DIR)/dummy.png
# break
# }
# }
Copy link
Member

Choose a reason for hiding this comment

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

open should not write files as a side effect. This should be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, but how else to precalculate this?

Copy link
Member

Choose a reason for hiding this comment

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

Does it actually save time? Most of the effort is to get sta warmed up. Drawing the tree should be cheap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, last I checked.

However, I can delete the commented code and if this PR is otherwise good to go, we can square away what we already have a solution for and I can then create a new PR with a narrower concern and some numbers.

A way to precalculate this that does not modify GUI state and also does not write files to disk is needed, which isnt available yet, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if there really is a need. I would be curious to see the time saved.

delete clock tree commented out code for now,
revisit later.

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@oharboe oharboe requested a review from maliberty December 18, 2024 11:42
@oharboe
Copy link
Collaborator Author

oharboe commented Dec 18, 2024

@maliberty Fixed, should be ready for merge now. This is how far we've come with prerendering for now. Will revisit more later in followup PRs.

}
}

set have_routes [expr {$placed && [grt::have_routes]}]
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to compute/check $placed? I thought this was fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes: there is no ppl::have_placed method. I can simplify the code a bit here though for the have_routes case.

}
puts "Prerendering $heatmap heatmap..."
gui::set_heatmap $heatmap rebuild 1
gui::dump_heatmap $heatmap $::env(REPORTS_DIR)/dummy.png
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid the file side effect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes: fixed

simplify now that grt::have_routes checks if placement
is done first.

no need to save dummy.png to prerender.

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@oharboe oharboe requested a review from maliberty December 18, 2024 19:39
@oharboe
Copy link
Collaborator Author

oharboe commented Dec 18, 2024

@maliberty Fixed, should be good to go.

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