Manual Moves Timing

[PaulaPerdomo]

Implementing manual moves feature which allows a user to specify a move in placement. If the move is legal, blocks are swapped and shown on the architecture.

Related Issue

• Previously, the manual moves feature was working, but slowing down the placement time by 6x-7x in comparison to the master branch. The increase in placement time was shown both on small and large circuits and generated failures in the vtr_reg_nightly tests for exceeding time and out of range.
• The placement time is now very similar/equal to the timing of the master branch. The delay was caused by a gtk function that was used to check the status of a toggle button. Since this function was called in a loop, the increase in time was significant. (As Prof. Betz suggested, the internal workings of the gtk function could have been performing a linear search or something similar that could have slowed down the code). The vtr_reg_nightly_tests passed after this change.

Motivation and Context

Optimizing the code to enable merge with master branch.

How Has This Been Tested?

Once the placement time was corrected, VPR output comparison were done between the master branch and the manual moves branch. This comparison shows the different outputs between the branches when --disp off: (These results are outdated) https://docs.google.com/spreadsheets/d/1PLCIUYsW7kX_kchS87_xRWioD9ZW5pa-36fpH6AoCaE/edit?usp=sharing.

vtr_reg_nightly1, 2, 3 were ran again and reported:

• vtr_reg_nightly_test1 had 0 QoR failures.
• vtr_reg_nightly_test2 had 4 QoR failures for being 0.09-0.126 above range from the master branch.
• vtr_reg_nightly_test3 had 1 QoR failure for being 0.02 below range.
  In the weekly meeting, these changes were not considered a problem.

The VTR and Titan Benchmarks were also ran and the GEOMEAN was calculated.
ManualMoves-Master Comparisons.xlsx

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
• All new and existing tests passed

[vaughnbetz]

As we discussed in the meeting today:

• Candidate slowdown: calling:
  bool manual_move_flag = get_manual_move_flag();
  might be slow. Even though it isn't much code, gtk may be doing a linear search or some such, so this may be slow enough to notice in a routine that is called many millions of times (which it is). Quick test: have this return false and see if the cpu time issue is gone.

• If that doesn't fix it, to get an idea of the slowdown, you'll need to do comparative testing of the master vs. your branch with timers, as absolute time numbers are hard to interpret for fast routines like this. You can also profile using the link Arash sent.

• You will need to do
  make format
  to pass the code format tests.

• To reduce diffs vs. the master you can rebase your branch onto the master. It will make it easier to review the PR

• You will need more commenting before this can be committed. Your new header file should comment exactly how to call each function and what it returns, and should comment every struct or data type defined. Using Doxygen format is best for function comments (I think we describe how to do that in the vtr developer documentation).

[vaughnbetz]

Also will need to avoid calling gtk code when graphics are off (--disp off)

[vaughnbetz]

@PaulaPerdomo : you have a warning in your code which will have to be fixed (no warnings allowed by CI):
In file included from /home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/draw/manual_moves.h:10,
from /home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/place.h:5,
from /home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/base/place_and_route.cpp:20:
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/move_generator.h:58:27: error: ‘virtual e_create_move MoveGenerator::propose_move(t_pl_blocks_to_be_moved&, e_move_type&, float, const t_placer_opts&, const PlacerCriticalities*)’ was hidden [-Werror=overloaded-virtual]
58 | virtual e_create_move propose_move(t_pl_blocks_to_be_moved& blocks_affected, e_move_type& /move_type/, float rlim, const t_placer_opts& placer_opts, const PlacerCriticalities* criticalities) = 0;
| ^~~~~~~~~~~~
In file included from /home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/place.h:5,
from /home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/base/place_and_route.cpp:20:
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/draw/manual_moves.h:49:19: note: by ‘e_create_move ManualMoveGenerator::propose_move(t_pl_blocks_to_be_moved&, float)’
49 | e_create_move propose_move(t_pl_blocks_to_be_moved& blocks_affected, float /rlim/);
| ^~~~~~~~~~~~
cc1plus: all warnings being treated as errors

You also have errors when graphics are disabled (-DNO_GRAPHICS compilation):
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/move_utils.cpp:455:5: error: ‘ManualMovesGlobals’ was not declared in this scope
455 | ManualMovesGlobals* manual_move_global = get_manual_moves_global();
| ^~~~~~~~~~~~~~~~~~
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/move_utils.cpp:455:25: error: ‘manual_move_global’ was not declared in this scope
455 | ManualMovesGlobals* manual_move_global = get_manual_moves_global();
| ^~~~~~~~~~~~~~~~~~
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/move_utils.cpp:455:46: error: ‘get_manual_moves_global’ was not declared in this scope
455 | ManualMovesGlobals* manual_move_global = get_manual_moves_global();
| ^~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/move_utils.cpp:460:13: error: ‘invalid_breakpoint_entry_window’ was not declared in this scope
460 | invalid_breakpoint_entry_window("Dimensions are out of bounds");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/move_utils.cpp:471:13: error: ‘invalid_breakpoint_entry_window’ was not declared in this scope
471 | invalid_breakpoint_entry_window("Blocks are not compatible");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/vpr/src/place/move_utils.cpp:480:17: error: ‘invalid_breakpoint_entry_window’ was not declared in this scope
480 | invalid_breakpoint_entry_window("Block is fixed");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[3]: *** [vpr/CMakeFiles/libvpr.dir/src/place/move_utils.cpp.o] Error 1
vpr/CMakeFiles/libvpr.dir/build.make:1209: recipe for target 'vpr/CMakeFiles/libvpr.dir/src/place/move_utils.cpp.o' failed

We need the program to compile properly without graphics, and some may not have the necessary packages to compile the graphics.

[vaughnbetz]

This looks like it's getting there. Final testing:

• sanitized strong CI test failed, but in a weird way (compiler internal error on odin and gen_fasm). Re-launched kokoro so hopefully it will pass now.
• @PaulaPerdomo : need to

Figure out why placement changes with graphics off on some Titan designs:
0. Attach the diff of the placement output for gsm_switch_stratixiv so we can see the first point where it differs.

1. Set a breakpoint in all your routines / code and run with graphics off through gdb, making sure none of your code is called (breakpoint not hit).
2. Run the sanitized build with --place (will skip other stages) on this design. Will check placer's memory.
3. Can run a different seed with --seed 5 on gsm_switch_stratixiv to see if the result is consistently worse than master, or just different.

Other testing:

1. Attach the VTR and Titan QoR data with graphics off
2. Run both a VTR and a Titan circuit in the graphics, click on all sorts of stuff and summarize how it works. Remember to test invalid input / crazy things (like moving a LAB to a RAM, moving a carry chain so part would go off the chip, etc.)
3. Build the sanitized vtr version, and run a small circuit through the graphics to confirm no memory errors.

Please summarize all the test results here as you get them.

[PaulaPerdomo]

@vaughnbetz @MohamedElgammal. Hello, I ran the gsm_switch_stratixiv architecture individually on the manual moves and master branch. The vpr.out file for both have the same placement. The temperature, Av cost, Av BB cost, Av TD cost and CPD is all the same. The difference is in the timing, where the manual move branch is slightly slower. I also compared the .place file and they were identical. From the parse_results.txt from both branch, the placed_CPD_est time is now the same, where before it increased by a factor of 0.16.

I changed my code this morning when I was testing everything a user should/shouldn't do, in case they activate the manual move toggle button but then decide not to use it. I'm not entirely sure yet if this could have caused a big difference or how this code was running since it was --disp off. I will test another circuit that had different placed_CPD_est, and compare its results too.

I attached the vpr.out file and .place files for both branches (in .txt format) :

gsm_switch_stratixiv_arch_timing_place_mm.txt
gsm_switch_stratixiv_arch_timing_place_master.txt

vpr_manual_moves.txt
vpr_master.txt

[vaughnbetz]

Thanks Paula. That's very good news. I suggest re-running all the Titan designs (you can use the regtest) and producing a spreadsheet to see if all the critical paths now line up (hopefully they do!).

[MohamedElgammal]

There are so many formatting edits that split a line into multiple lines. You might be using an IDE and setting the text wrap option to a small values which causes all these edits.

[vaughnbetz]

@PaulaPerdomo : you should get rid of the short lines in that case, and rely on
make format
to adjust the final format.

Or are the formatting changes due to make format (in which case they're fine, as we rely on make format everywhere).

[MohamedElgammal]

Thanks for getting this working. Please mention me when you add the full Titan results after the fix.

The code seems to be well organized. I have left some comments to be addressed. My main comment is that this PR includes many formatting changes to old code (not related to manual move) and some of them are actually making the code less readable. It seems that these edits were done automatically by an IDE.

[vaughnbetz]

Thanks for all the code updates @PaulaPerdomo . They look good.
There are a few left -- for example there are two places in place.cpp where I think helper functions for manual moves would shorten the code, and in manual_moves.h the comment of which block the user selected should say "used as the from block in the move generator". If you can mark the comments that are resolved as such by hitting the resolve conversation button it will help us see what's left.

Pasting in some info from an email from Paula on the cpd results changing on the manual moves branch:

I updated my code based on the code changes requested by Mohamed and you and are ready to be reviewed again. I also did the Manual moves documentation and the branch is called "documentation_manual_moves".

Also, the QoR results between both branches is not the same for all placed_CPD_est. There are like 6 architectures where the results don't match with the master branch. I have gone through the code with gdb, checked for sanitizers leaks and I have not found any segmentation faults or memory leaks reported when doing so. I also changed the seed number, and the results are varied by very little when doing this. I don't know why there are changes to this number and they vary from the master branch by 1-2 seconds in some architectures. However, when I run a circuit individually (instead of the whole titan benchmarks), these results match the master branch which I found odd. According to the VPR output file, the changes in placement happen ever since the first move with the circuits that don't match their values with the master branch.

[vaughnbetz]

This is almost ready to merge. @PaulaPerdomo if you can address the remaining comments (see above) we can merge this.
For the cpd delay changes, can you call print_place to dump the initial placement before any moves have happened, and then diff it on the master vs. the manual_moves branch. If that is different then the change would appear to be outside your code, so we could stop looking at your code for it.

[vaughnbetz]

@PaulaPerdomo : can you do a pull request on the documentation branch to get that in too?

[vaughnbetz]

Everything passed except nightly tests 1, which might be hung (started just before midnight last night). Will wait until this evening and then restart kokoro if necessary.

[vaughnbetz]

Thanks @PaulaPerdomo. I see the documentation and helper function are added. Anything more to do from a coding perspective to merge this?
@MohamedElgammal : any luck in tracing what might / might not be causing the cpd changes?
Once CI finishes, I am tempted to merge this and then @MohamedElgammal can keep evaluating where the cpd change comes from. Any objections?

[vaughnbetz]

Thanks @PaulaPerdomo for working on this after the official end of your summer to get it in. Merging it!