Refactor the placer debug code #1522

MohamedElgammal · 2020-09-02T23:30:34Z

Description

This PR follows #1287 . In this PR, some code refactoring was done while preserving the same functionality.

Related Issue

Motivation and Context

Taking into consideration some code review comments on #1287

How Has This Been Tested?

test that the UI is working for multiple VPR benchmarks
test that the feature is disabled if the compilation or the code flag is disabled

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
[ x] All new and existing tests passed

…ve duplicated code

vaughnbetz · 2020-09-04T17:20:55Z

vpr/src/draw/draw.cpp

@@ -998,16 +998,16 @@ static void drawplace(ezgl::renderer* g) {
                ezgl::color block_color;
                t_logical_block_type_ptr logical_block_type = nullptr;
 #    ifdef VTR_ENABLE_DEBUG_LOGGING


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.

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

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.

vaughnbetz · 2020-09-04T17:29:18Z

vpr/src/draw/draw.cpp

@@ -998,16 +998,16 @@ static void drawplace(ezgl::renderer* g) {
                ezgl::color block_color;


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

vaughnbetz · 2020-09-04T17:31:42Z

vpr/src/draw/draw.cpp

+                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); });


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

vaughnbetz · 2020-09-04T17:33:40Z

vpr/src/draw/draw.cpp

+                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 (placer_debug_enabled() && it != draw_state->colored_blocks.end()) {


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

vaughnbetz · 2020-09-04T17:40:15Z

vpr/src/draw/draw.cpp

+                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 (placer_debug_enabled() && it != draw_state->colored_blocks.end()) {


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

vaughnbetz · 2020-09-04T17:40:34Z

vpr/src/draw/draw.cpp

+                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 (placer_debug_enabled() && it != draw_state->colored_blocks.end()) {
                        block_color = it->second;
                        auto tile_type = device_ctx.grid[i][j].type;
                        logical_block_type = pick_best_logical_type(tile_type);
                    } else {


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

vaughnbetz · 2020-09-04T17:41:36Z

vpr/src/draw/draw.cpp

+// location highlighting functions 
+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));    


change variable name to colored_locs

vaughnbetz · 2020-09-04T17:42:50Z

vpr/src/draw/draw.cpp

@@ -4157,4 +4135,16 @@ static void run_graphics_commands(std::string commands) {
    ++draw_state->sequence_number;
 }

+#ifdef VTR_ENABLE_DEBUG_LOGGING
+// location highlighting functions 


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.

vaughnbetz · 2020-09-04T17:43:17Z

vpr/src/draw/draw.cpp

+    t_draw_state* draw_state = get_draw_state_vars();
+    draw_state->colored_blocks.push_back(std::make_pair(loc,clr));    
+}
+void clear_colored_blocks(){


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

vaughnbetz · 2020-09-04T17:47:17Z

vpr/src/draw/draw.h

@@ -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


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

vaughnbetz · 2020-09-04T17:47:34Z

vpr/src/draw/draw.h

@@ -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
+// location highlighting functions 
+void set_draw_loc_color(t_pl_loc , ezgl::color );                                                                                  


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

vaughnbetz · 2020-09-04T17:48:55Z

vpr/src/place/move_utils.cpp

@@ -8,9 +8,8 @@
 //Run-time flag to control when placer debug information is printed


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).

vaughnbetz · 2020-09-04T17:50:17Z

vpr/src/place/move_utils.h

@@ -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


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

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

vaughnbetz · 2020-09-04T17:51:16Z

vpr/src/place/place.cpp

@@ -45,13 +45,17 @@
 #include "tatum/TimingReporter.hpp"

 #ifdef VTR_ENABLE_DEBUG_LOGGING


Get rid of ifdef if possible.

vaughnbetz · 2020-09-04T17:51:29Z

vpr/src/place/place.cpp

 #    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


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

vaughnbetz · 2020-09-04T17:54:41Z

vpr/src/place/place.cpp

@@ -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)); 


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 {

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.

vaughnbetz · 2020-09-04T17:57:29Z

vpr/src/place/place.cpp

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

 #ifdef VTR_ENABLE_DEBUG_LOGGING


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.

Looking closer, I think Mahshad already did this.

vaughnbetz · 2020-09-04T18:00:48Z

vpr/src/place/place.cpp

@@ -3067,6 +3071,10 @@ void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affect



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).

vaughnbetz · 2020-09-04T18:02:03Z

Various inline comments above ... please take a crack at them and let me know when it's ready for merging.

- all the drawing code is moved to draw.cpp - the breakpoint algorithm is moved from place.cpp to a new file - the compilation debugging guard is only used when needed - Using more meaningful names - Added high level comments for all placer debugging functions

MohamedElgammal · 2020-09-06T01:22:30Z

@vaughnbetz , I have pushed a new commit that addresses all your comments. please let me know if there are any further comments.

…n VPR_USE_EZGL is disabled

vaughnbetz · 2020-09-10T17:11:26Z

Looks good. The QoR failure was just a golden result that needed updating. Merging.

MohamedElgammal added 3 commits September 2, 2020 12:08

remove f_placer_debug global variable, wrap it with accessor and remo…

78934b0

…ve duplicated code

add setter/accessor for f_place_debug

66aa7a1

decouple the placement and drawing code

a1f53f6

probot-autolabeler bot added lang-cpp C/C++ code VPR VPR FPGA Placement & Routing Tool labels Sep 2, 2020

MohamedElgammal requested a review from vaughnbetz September 2, 2020 23:31

MohamedElgammal changed the title ~~Place debug refactor~~ Refactor the placer debug code Sep 2, 2020

vaughnbetz reviewed Sep 4, 2020

View reviewed changes

MohamedElgammal requested a review from vaughnbetz September 6, 2020 01:24

adding NO_GRAPHICS compilation guard to prevent compilation error whe…

2a687f0

…n VPR_USE_EZGL is disabled

MohamedElgammal added 2 commits September 5, 2020 22:39

code formatting

0c7fd43

Adding two new regtests with NO_GRAPHICS and VTR_ENABLE_DEBUGL_LOGGING

a1d3e9d

probot-autolabeler bot added the infra Project Infrastructure label Sep 6, 2020

MohamedElgammal added 3 commits September 6, 2020 00:14

add a missing include file

bc12a5a

Add backward compitability for GTK older than 3.12

2f2a08b

code formatting

fb6cda3

vaughnbetz merged commit 1478410 into master Sep 10, 2020

vaughnbetz deleted the place_debug_refactor branch September 10, 2020 17:11

		@@ -998,16 +998,16 @@ static void drawplace(ezgl::renderer* g) {
		ezgl::color block_color;

		@@ -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

		@@ -8,9 +8,8 @@
		//Run-time flag to control when placer debug information is printed

		@@ -45,13 +45,17 @@
		#include "tatum/TimingReporter.hpp"

		#ifdef VTR_ENABLE_DEBUG_LOGGING

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

		#ifdef VTR_ENABLE_DEBUG_LOGGING

		@@ -3067,6 +3071,10 @@ void stop_placement_and_check_breakopints(t_pl_blocks_to_be_moved& blocks_affect

Refactor the placer debug code #1522

Refactor the placer debug code #1522

Uh oh!

Conversation

MohamedElgammal commented Sep 2, 2020

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 4, 2020

Uh oh!

MohamedElgammal commented Sep 6, 2020

Uh oh!

vaughnbetz commented Sep 10, 2020

Uh oh!

Uh oh!