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

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?"

flow/scripts/open.tcl Outdated Show resolved Hide resolved
@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 <[email protected]>
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 <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
@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.

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 30, 2024

good to go?

@maliberty
Copy link
Member

I think all this warming should have its own control and be off by default. I don't want to spend time on all this if I know what I'm going to be looking at is a subset.

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 30, 2024

I think all this warming should have its own control and be off by default. I don't want to spend time on all this if I know what I'm going to be looking at is a subset.

Would you be happy with a policy to unconditionally compute it at build time?

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 30, 2024

Meaning: would you agree that it is better to close this PR and then open a more longer term feature request of storing precomputed values unconditionally in the .odb file?

@oharboe
Copy link
Collaborator Author

oharboe commented Dec 30, 2024

Every option halves the usefulness of a feature.

@maliberty
Copy link
Member

This is an unconditional penalty on the user who isn't interested in "global warming" 😉

I would be a very long term / large enhancement to store all the sta data in odb. I'm ok if you want to open it but expect the response to be very slow.

@oharboe
Copy link
Collaborator Author

oharboe commented Jan 2, 2025

@maliberty broke out The-OpenROAD-Project/OpenROAD#6457 and only the timing reports are now updated, but only if GUI_TIMING=1

@maliberty maliberty merged commit 279d9c7 into The-OpenROAD-Project:master Jan 2, 2025
6 checks passed
@oharboe oharboe deleted the make-gui-less-click-and-wait branch January 3, 2025 06:16
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