Subtile Pin Location

[amin1377]

In the architecture file, when a sub-tile has a capacity bigger than one, it is assumed that the pin location of all sub-tiles should be assigned together:

<tile name="io">
      <sub_tile name="io" capacity="2">
       ....
             <pinlocations pattern="custom">
                      <loc side="left">  io.core_in[0:24]  io.core_out[0:19]  io.clk[0:2]</loc>
       ....

In the above case, the first half of the IO pins for both sub-tiles will be placed on the left side of the device.

In this PR, a feature from OpenFPGA is added where one can define different locations for each sub-tile capacity if necessary:

<tile name="io">
      <sub_tile name="io" capacity="2">
       ....
             <pinlocations pattern="custom">
                      <loc side="left">  io[0:0].core_in[0:24]  io[0:0].core_out[0:19]  io[1:1].clk[0:2]</loc>
       ....

[AlexandreSinger]

This looks good to me other than some minor comments. I am not as familiar with this code, but I gave some general comments.

Can you please add some documentation for this new feature to the architecture docs? I think you can just directly copy the docs from OpenFPGA.

[AlexandreSinger]

Although it is not "required", I think freeTokens should be called before all of the throw statements in the function for cleanliness. I have run into issues in the past where throws fail due to memory not being deallocated properly.

[amin1377]

Interesting! I added a helper function throw_xml_arch_error that takes the tokens and frees them before throwing an error. I replaced all archfpga_throw calls with this helper function (it took me a while :) )

static void throw_xml_arch_error(const char* filename, 
                                  int line, 
                                  const std::string& err_msg, 
                                  t_token* tokens, 
                                  int num_tokens);

[soheilshahrouz]

Most function names in this file are camel case, but the rest of code base uses snake case. We should gradually change this. NoC-realted function names are snake case. I think new functions should use snake case too.

Why is this a template function? It is only called once with T=t_sub_tile*. I also suggest we pass it by const reference instead of pointer.

Add doxygen comment for this function

[amin1377]

I changed the name of all functions in this file from camel case to snake case.

[soheilshahrouz]

You can use auto to unpack the pair and give each of its members a name. For example:
auto [capacity_range_low, capacity_range_high] = ...

[amin1377]

Look who’s suddenly in favor of using auto :)) (just kidding)

[soheilshahrouz]

Thanks for this new feature.
I think it should be also documented in the architecture reference.

[amin1377]

@soheilshahrouz @AlexandreSinger: Thanks for reviewing this PR. I’ve applied your comments. Please let me know if you have any further suggestions.

[AlexandreSinger]

LGTM. The throw function freeing memory makes sense, but its a bit unclear to someone using the code that they need to do that; however I know that Soheil is working on cleaning up the token code to prevent this so it does not matter.

I still think it would be good to document this change!

[amin1377]

@AlexandreSinger:

I added the following Doxygen comment for throw_xml_arch_error :

/**
 * @brief Throws an error with a message and frees the tokens
 *
 * @param filename File name associated with the error
 * @param line Line number associated with the error
 * @param err_msg Error message to be displayed
 * @param tokens If tokens are allocated before calling this function,
 * they will be freed here. If not, nullptr should be passed.
 * @param num_tokens Number of tokens to be freed. If not, 0 should be passed.
 */

Also, inside of the function I added the following comment:

// We need to free the tokens here, otherwise,
// archfpga_throw will face issues since memory is
// not deallocated properly.
```.

[amin1377]

@AlexandreSinger: Added the following to architecture documentation:

If the subtile capacity is greater than 1, you can specify the capacity range when defining the pin locations. For example:

        .. code-block:: xml

            <sub_tile name="io_bottom" capacity="6">
                <equivalent_sites>
                    <site pb_type="io"/>
                </equivalent_sites>
                <input name="outpad" num_pins="1"/>
                <output name="inpad" num_pins="1"/>
                <fc in_type="frac" in_val="0.15" out_type="frac" out_val="0.10"/>
                <pinlocations pattern="custom">
                    <loc side="top">io_bottom[0:1].outpad io_bottom[0:3].inpad io_bottom[2:5].outpad io_bottom[4:5].inpad</loc>
                </pinlocations>
            </sub_tile>
        
        If no capacity range is specified, it is assumed that the location applies to all capacity instances.

[AlexandreSinger]

LGTM thanks for adding the documentation!

[soheilshahrouz]

@amin1377 Thanks for all these changes!

As discussed yesterday, I created a class to avoid using raw pointers in vtr_tokens. Is it possible to revert changes you made to vtr_tokens to avoid merge confilicts? If not, I'll resolve them later.

[amin1377]

@soheilshahrouz: Sure. I'll revert the commit.