Skip to content

Refactor the placer debug code #1522

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Sep 10, 2020
Merged

Refactor the placer debug code #1522

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
78934b0
remove f_placer_debug global variable, wrap it with accessor and remo…
Sep 2, 2020
66aa7a1
add setter/accessor for f_place_debug
Sep 2, 2020
a1f53f6
decouple the placement and drawing code
Sep 2, 2020
e7b1168
Refactor and addressed code review comments
Sep 6, 2020
2a687f0
adding NO_GRAPHICS compilation guard to prevent compilation error whe…
Sep 6, 2020
0c7fd43
code formatting
Sep 6, 2020
a1d3e9d
Adding two new regtests with NO_GRAPHICS and VTR_ENABLE_DEBUGL_LOGGING
Sep 6, 2020
bc12a5a
add a missing include file
Sep 6, 2020
2f2a08b
Add backward compitability for GTK older than 3.12
Sep 6, 2020
fb6cda3
code formatting
Sep 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 20 additions & 30 deletions vpr/src/draw/draw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,16 +998,16 @@ static void drawplace(ezgl::renderer* g) {
ezgl::color block_color;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put the continue for if (bnum == ...) on a separate, indented line.

t_logical_block_type_ptr logical_block_type = nullptr;
# ifdef VTR_ENABLE_DEBUG_LOGGING
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this guard? Does it only control the breakpoint functionality or something else? Does compiling in breakpoints slow the tool down?
If not, we can just eliminate this #ifdef throughout the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd factor this into a subroutine; shorter routines with meaningful names are easier to understand and change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once in a subroutine, you should comment the function (above the function header) about exactly what it does. Add inline comments for anything tricky or non-obvious if necessary.

if (f_placer_debug) {
t_pl_loc curr_loc;
curr_loc.x = i;
curr_loc.y = j;
auto it = std::find_if(draw_state->colored_blocks.begin(), draw_state->colored_blocks.end(), [&curr_loc](const std::pair<t_pl_loc, ezgl::color>& a) { return (a.first.x == curr_loc.x && a.first.y == curr_loc.y); });
if (it != draw_state->colored_blocks.end()) {
t_pl_loc curr_loc;
curr_loc.x = i;
curr_loc.y = j;
auto it = std::find_if(draw_state->colored_blocks.begin(), draw_state->colored_blocks.end(), [&curr_loc](const std::pair<t_pl_loc, ezgl::color>& a) { return (a.first.x == curr_loc.x && a.first.y == curr_loc.y); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a better name than a.
colored_blocks should really be called colored_locations (I think).

if (placer_debug_enabled() && it != draw_state->colored_blocks.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is placer_debug_enabled() the right name? Does this really mean we are in a breakpoint? If so worth renaming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: found a colored location at the spot I am drawing (currently used for drawing the current move). This overrides any block color.

block_color = it->second;
auto tile_type = device_ctx.grid[i][j].type;
logical_block_type = pick_best_logical_type(tile_type);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

No color specified at this location; use the block color.

# endif
if (bnum != EMPTY_BLOCK_ID) {
block_color = draw_state->block_color(bnum);
logical_block_type = cluster_ctx.clb_nlist.block_type(bnum);
Expand All @@ -1018,31 +1018,9 @@ static void drawplace(ezgl::renderer* g) {
auto tile_type = device_ctx.grid[i][j].type;
logical_block_type = pick_best_logical_type(tile_type);
}
}
} else {
if (bnum != EMPTY_BLOCK_ID) {
block_color = draw_state->block_color(bnum);
logical_block_type = cluster_ctx.clb_nlist.block_type(bnum);
} else {
block_color = get_block_type_color(device_ctx.grid[i][j].type);
block_color = lighten_color(block_color, EMPTY_BLOCK_LIGHTEN_FACTOR);

auto tile_type = device_ctx.grid[i][j].type;
logical_block_type = pick_best_logical_type(tile_type);
}
# ifdef VTR_ENABLE_DEBUG_LOGGING
}
# else
if (bnum != EMPTY_BLOCK_ID) {
block_color = draw_state->block_color(bnum);
logical_block_type = cluster_ctx.clb_nlist.block_type(bnum);
} else {
block_color = get_block_type_color(device_ctx.grid[i][j].type);
block_color = lighten_color(block_color, EMPTY_BLOCK_LIGHTEN_FACTOR);

auto tile_type = device_ctx.grid[i][j].type;
logical_block_type = pick_best_logical_type(tile_type);
}
# endif
# endif
g->set_color(block_color);
/* Get coords of current sub_tile */
ezgl::rectangle abs_clb_bbox = draw_coords->get_absolute_clb_bbox(i, j, k, logical_block_type);
Expand Down Expand Up @@ -4157,4 +4135,16 @@ static void run_graphics_commands(std::string commands) {
++draw_state->sequence_number;
}

#ifdef VTR_ENABLE_DEBUG_LOGGING
// location highlighting functions
Copy link
Contributor

Choose a reason for hiding this comment

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

Expand this: pass in an (x,y,subtile) location and the color in which it should be drawn. This overrides the color of any block placed in that location, and also applies if the location is empty.

void set_draw_loc_color(t_pl_loc loc, ezgl::color clr){
t_draw_state* draw_state = get_draw_state_vars();
draw_state->colored_blocks.push_back(std::make_pair(loc,clr));
Copy link
Contributor

Choose a reason for hiding this comment

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

change variable name to colored_locs

}
void clear_colored_blocks(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line (vertical spacing).
Add a function level comment.

t_draw_state* draw_state = get_draw_state_vars();
draw_state->colored_blocks.clear();
}
#endif

#endif /* NO_GRAPHICS */
6 changes: 6 additions & 0 deletions vpr/src/draw/draw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ float get_net_alpha();

ezgl::color get_block_type_color(t_physical_tile_type_ptr type);

#ifdef VTR_ENABLE_DEBUG_LOGGING
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the #ifdef (unless you show a slowdown).

// location highlighting functions
void set_draw_loc_color(t_pl_loc , ezgl::color );
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the function level comments (or duplicate them) here.

void clear_colored_blocks();
#endif

#endif /* NO_GRAPHICS */

#endif /* DRAW_H */
12 changes: 10 additions & 2 deletions vpr/src/place/move_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@
//Run-time flag to control when placer debug information is printed
Copy link
Contributor

Choose a reason for hiding this comment

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

First two lines of this comment can probably be deleted. (get rid of #ifdef and second line seems less detailed / accurate than the rest of the comment).

//Note : only enables debug output if compiled with VTR_ENABLE_DEBUG_LOGGING is defined
//f_placer_debug is used to stop the placer when a breakpoint is reached. When this flag is true, it stops the placer after the current perturbation. Thus, when a breakpoint is reached, this flag is set to true.
#ifdef VTR_ENABLE_DEBUG_LOGGING
bool f_placer_debug = false;
#endif

//Records counts of reasons for aborted moves
static std::map<std::string, size_t> f_move_abort_reasons;

Expand Down Expand Up @@ -657,3 +656,12 @@ bool find_to_loc_uniform(t_logical_block_type_ptr type,

return true;
}

//Accessor for f_placer_debug
bool placer_debug_enabled() {
return f_placer_debug;
}

void set_placer_debug(bool flag){
f_placer_debug = flag;
}
10 changes: 4 additions & 6 deletions vpr/src/place/move_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,6 @@
#include "move_transactions.h"
#include "compressed_grid.h"

//Run-time flag to control when placer debug information is printed
//Note : only enables debug output if compiled with VTR_ENABLE_DEBUG_LOGGING is defined
//f_placer_debug is used to stop the placer when a breakpoint is reached. When this flag is true, it stops the placer after the current perturbation. Thus, when a breakpoint is reached, this flag is set to true.
#ifdef VTR_ENABLE_DEBUG_LOGGING
extern bool f_placer_debug;
#endif

/* This is for the placement swap routines. A swap attempt could be *
* rejected, accepted or aborted (due to the limitations placed on the *
Expand Down Expand Up @@ -57,4 +51,8 @@ bool find_to_loc_uniform(t_logical_block_type_ptr type,
float rlim,
const t_pl_loc from,
t_pl_loc& to);

//Accessor and setter for f_placer_debug
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain what setting this to true or false means. (idea: person who wants to call these should only have to read the .h file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is placer_debug_enabled the right name? Or should it be placer_breakpoint_reached? (or something like that).

bool placer_debug_enabled();
void set_placer_debug(bool );
#endif
22 changes: 15 additions & 7 deletions vpr/src/place/place.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,17 @@
#include "tatum/TimingReporter.hpp"

#ifdef VTR_ENABLE_DEBUG_LOGGING
Copy link
Contributor

Choose a reason for hiding this comment

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

Get rid of ifdef if possible.

# include "draw.h"
# include "draw_types.h"
# include "draw_global.h"
# include "draw_color.h"
# include "breakpoint.h"
//map of the available move types and their corresponding type number
std::map<int, std::string> available_move_types = {
{0, "Uniform"}};

#define OLD_BLK_LOC_COLOR blk_GOLD
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these colors be defined in a draw routine?

#define NEW_BLK_LOC_COLOR blk_GREEN
#endif

using std::max;
Expand Down Expand Up @@ -508,7 +512,7 @@ static void print_resources_utilization();

void transform_blocks_affected(t_pl_blocks_to_be_moved blocksAffected);
static void init_annealing_state(t_annealing_state* state, const t_annealing_sched& annealing_sched, float t, float rlim, int move_lim_max, float crit_exponent);
void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affected, bool& f_place_debug, e_move_result move_outcome, double delta_c, double bb_delta_c, double timing_delta_c);
void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affected, e_move_result move_outcome, double delta_c, double bb_delta_c, double timing_delta_c);

/*****************************************************************************/
void try_place(const t_placer_opts& placer_opts,
Expand Down Expand Up @@ -1537,7 +1541,7 @@ static e_move_result try_swap(float t,
move_generator.process_outcome(move_outcome_stats);

#ifdef VTR_ENABLE_DEBUG_LOGGING
stop_placement_and_check_breakopints(blocks_affected, f_placer_debug, move_outcome, delta_c, bb_delta_c, timing_delta_c);
stop_placement_and_check_breakopints(blocks_affected, move_outcome, delta_c, bb_delta_c, timing_delta_c);
#endif
clear_move_blocks(blocks_affected);

Expand Down Expand Up @@ -3037,7 +3041,7 @@ void transform_blocks_affected(t_pl_blocks_to_be_moved blocksAffected) {
}

#ifdef VTR_ENABLE_DEBUG_LOGGING
Copy link
Contributor

Choose a reason for hiding this comment

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

Minimize added code in try_swap (already a critical routine and overly long). Create high-level functions, if possible in a new place_debug.cpp (or some such) file and call them from here. Should only see a few calls / condition checks in try_swap for breakpoints and they should be well commented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking closer, I think Mahshad already did this.

void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affected, bool& f_place_debug, e_move_result move_outcome, double delta_c, double bb_delta_c, double timing_delta_c) {
void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affected, e_move_result move_outcome, double delta_c, double bb_delta_c, double timing_delta_c) {
t_draw_state* draw_state = get_draw_state_vars();
if (draw_state->list_of_breakpoints.size() != 0) {
//update current information
Expand All @@ -3046,13 +3050,13 @@ void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affect
get_bp_state_globals()->get_glob_breakpoint_state()->from_block = size_t(blocks_affected.moved_blocks[0].block_num);

//check for breakpoints
f_place_debug = check_for_breakpoints(true);
if (f_place_debug)
set_placer_debug(check_for_breakpoints(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you should do something like:
if (check_for_breakpoints (true)) { // What does the true flag passed in mean? Comment this.
set_placer_debug (true);
....
else {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reviewing the code, I found that the true flag means that the function is called from placer. I added a comment by this meaning.

if (placer_debug_enabled())
breakpoint_info_window(get_bp_state_globals()->get_glob_breakpoint_state()->bp_description, *get_bp_state_globals()->get_glob_breakpoint_state(), true);
} else
f_place_debug = false;
set_placer_debug(false);

if (f_place_debug && draw_state->show_graphics) {
if (placer_debug_enabled() && draw_state->show_graphics) {
std::string msg = available_move_types[0];
if (move_outcome == 0)
msg += vtr::string_fmt(", Rejected");
Expand All @@ -3067,6 +3071,10 @@ void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affect

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider if this routine should be moved to another file, in /draw or in a placer breakpoints .cpp file (can't recall the exact names).

deselect_all();
draw_highlight_blocks_color(cluster_ctx.clb_nlist.block_type(blocks_affected.moved_blocks[0].block_num), blocks_affected.moved_blocks[0].block_num);

clear_colored_blocks();
set_draw_loc_color(blocks_affected.moved_blocks[0].old_loc, OLD_BLK_LOC_COLOR);
set_draw_loc_color(blocks_affected.moved_blocks[0].old_loc, NEW_BLK_LOC_COLOR);
draw_state->colored_blocks.clear();

draw_state->colored_blocks.push_back(std::make_pair(blocks_affected.moved_blocks[0].old_loc, blk_GOLD));
Expand Down