[TestCoverage] Failing VTR Benchmarks on the 7Series Architecture

[AlexandreSinger]

Within VTR, we have 3 tests which are testing the 7series architecture (one nightly test and two strong tests):

1. https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/tasks/regression_tests/vtr_reg_nightly_test2/vtr_xilinx_qor/config/config.txt
2. https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/tasks/regression_tests/vtr_reg_strong/strong_xilinx_support/config/config.txt
3. https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vtr_flow/tasks/regression_tests/vtr_reg_strong_odin/strong_xilinx_support/config/config.txt

The issue is that none of these tests are actually running on CI since none of them were added to the appropriate task lists. As such, they appear to have regressed.

The two strong tests are actually ill-formed. It looks like they got mangled in an upgrade and lost their testcases. I believe they were just quick tests that tested on stereovision3, but I am not sure.

The NightlyTest2 test is the most important one. It runs the full (I think) 7series architecture on 6 large VTR benchmarks. I tried to revive this testcase on the CI, however all 6 of these circuits are failing.

All 6 circuits are failing at the same point in the Prepacking stage:

The issue appears to be within the following function:

vtr-verilog-to-routing/vpr/src/pack/prepack.cpp

Lines 1620 to 1642 in 45945b8

	static void update_chain_root_pins(t_pack_patterns* chain_pattern,
	const std::vector<t_pb_graph_pin*>& chain_input_pins) {
	std::vector<std::vector<t_pb_graph_pin*>> primitive_input_pins;
	std::unordered_set<t_pb_type*> pattern_blocks = get_pattern_blocks(*chain_pattern);
	for (const auto pin_ptr : chain_input_pins) {
	std::vector<t_pb_graph_pin*> connected_primitive_pins;
	get_all_connected_primitive_pins(pin_ptr, pattern_blocks, connected_primitive_pins);
	/**
	* It is required that the chain pins are connected inside a complex
	* block. Although it is allowed to have them disconnected in some
	* modes of the block provided that there is always at least one mode
	* that has them connected inside. The following assert checks for
	* that.
	*/
	VTR_ASSERT(connected_primitive_pins.size());
	primitive_input_pins.push_back(connected_primitive_pins);
	}
	chain_pattern->chain_root_pins = primitive_input_pins;
	}

This seems to be related to a recent PR which changed the get_all_connected_pins function:

#3007

To debug this, we may need to view the version just before this PR was merged and try to figure out what changed and why it is not finding any connected primitive pins for this architecture.

[AlexandreSinger]

@vaughnbetz FYI
@amin1377 FYI

[amin1377]

Thanks for pointing this out, Alex. I’ll look into the issue.

[AlexandreSinger]

I am planning on writing a script that will check all of our task lists and ensure that all tasks in the folders are found in the correct task list. I will add a feature to ignore some tests just to get this in, but we really need to ensure that tasks that are added to VTR are actually being run.

[amin1377]

we really need to ensure that tasks that are added to VTR are actually being run.

Couldn't agree more!

[vaughnbetz]

Agreed! Adding @WhiteNinjaZ to add these tests into the appropriate nightly and strong suits and help debug any further issues.

[WhiteNinjaZ]

Will do

[WhiteNinjaZ]

Just added tests to CI. It looks like the issues are coming out of the adder chains in the xilinx arch. The way these are set up: there are two slices per CLB and each slice has four 1-bit adders with carry. The interconnect for these chains is as follows:

<pb_type name="CLB">
   ...
   <pb_type name="SLICE_L" num_pb="2">
       ...
      <mode name="independent_LUTs">
         <pb_type name="fle" num_pb="4">
          ...
            <pb_type blif_model=".subckt adder" name="adder" num_pb="1">
              <input name="a" num_pins="1" />
              <input name="b" num_pins="1" />
              <input name="cin" num_pins="1" />
              <output name="cout" num_pins="1" />
              <output name="sumout" num_pins="1" />
              ...
             </pb_type>
            <interconnect>
              <mux name="cin" input="fle.cin fle.inX" output="adder.cin">
                <pack_pattern name="chain" in_port="fle.cin" out_port="adder.cin" />
                <pack_pattern name="chain2" in_port="fle.inX" out_port="adder.cin" />
              </mux>
              <direct name="cout" input="adder.cout" output="fle.cout">
                <pack_pattern name="chain" in_port="adder.cout" out_port="fle.cout" />
              </direct>
             ...
           </interconnect>
        </pb_type>
        <interconnect>
            <!-- Adder chain internal to slice -->
            <direct name="cin" input="SLICE_L.cin" output="fle[0].cin">
              <pack_pattern name="chain" in_port="SLICE_L.cin" out_port="fle[0].cin" />
            </direct>
            <direct name="Acarry" input="fle[0].cout" output="fle[1].cin">
              <pack_pattern name="chain" in_port="fle[0].cout" out_port="fle[1].cin" />
            </direct>
            <direct name="Bcarry" input="fle[1].cout" output="fle[2].cin">
              <pack_pattern name="chain" in_port="fle[1].cout" out_port="fle[2].cin" />
            </direct>
            <direct name="Ccarry" input="fle[2].cout" output="fle[3].cin">
              <pack_pattern name="chain" in_port="fle[2].cout" out_port="fle[3].cin" />
            </direct>
            <direct name="cout" input="fle[3].cout" output="SLICE_L.cout">
              <pack_pattern name="chain" in_port="fle[3].cout" out_port="SLICE_L.cout" />
            </direct>
            ...
        </interconnect>
   </pb_type>
   <interconnect>
        <!-- The follwing Cin/Cout pattern matches xilinx. This chain is connect to the output pins of the CLB -->
        <direct name="cin1" input="CLB.cin[0]" output="SLICE_L[0:0].cin">
          <pack_pattern name="chain" in_port="CLB.cin[0]" out_port="SLICE_L[0:0].cin" />
        </direct>
        <direct name="cin2" input="CLB.cin[1]" output="SLICE_L[1:1].cin">
          <pack_pattern name="chain" in_port="CLB.cin[1]" out_port="SLICE_L[1:1].cin" />
        </direct>
        <direct name="cout1" input="SLICE_L[0:0].cout" output="CLB.cout[0]">
          <pack_pattern name="chain" in_port="SLICE_L[0:0].cout" out_port="CLB.cout[0]" />
        </direct>
        <direct name="cout2" input="SLICE_L[1:1].cout" output="CLB.cout[1]">
          <pack_pattern name="chain" in_port="SLICE_L[1:1].cout" out_port="CLB.cout[1]" />
        </direct>
   </interconnect>
</pb_type>
   

The complexity here lies in that the carry chains are barried within multiple layers of PB_types. It looks like CI is also failing because both SLICEL carry chains are named the same (i.e. SLICEL[0] and SLICEL[1]). I am looking into this.

[WhiteNinjaZ]

@amin1377 it looks like the change that broke the architecture was somewhere in #3007 as Alex has already stated. The addition of the 7-series to the CI tests is in PR #3120. I am also taking a look at the code as well as the architecture to try and figure out why things broke so let me know if there is anything I can do to help.

[WhiteNinjaZ]

@amin1377 I have been able to locate what is causing the 7-series to fail. It appears to be coming from this line in prepack.cpp that was added in the already mentioned PR:

if (pattern_blocks.find(output_edge->output_pins[ipin]->parent_node->pb_type) != pattern_blocks.end()) {
                    connected_primitive_pins.push_back(output_edge->output_pins[ipin]);
                }

Removing the above if statement remedies the issue. I am not entirely sure, but I think the root of the problem comes from the fact that the seven series has two slices each of which have an internal carry chain (commented as <-- Adder chain internal to slice --> in the architecture snippet above). The way the XML is set up, each slice has a cin/cout output that eventually connects to the topmost CLB cin/cout carry chain. Because the if statement above checks if the output pin is related to a top level pb_type either one or both of the internal chains to the slice are not being added to the connected_primitive_pins vector and thus the arch fails latter checks for connected chains. It might also be because these chains are connecting FLE's (which are internal to a slice) to the slice which then connects to the CLB (i.e. the connections are berried three layers deep).

[WhiteNinjaZ]

@amin1377 actually I just realized that there are two dangling pack patterns (on lines 984 and 959) in the 7-series arch itself. Removing those seems to fix the problem so it might not actually be an issue with the code changes. The Errors being thrown however where pointing to the adder carry chains, hence my confusion. Ill remove those two chains from the arch in my PR.

[amin1377]

@WhiteNinjaZ: Interesting! Thanks for letting me know.

[vaughnbetz]

If that fixes it, that would be good news!

[AlexandreSinger]

Issue was resolved in #3120