Skip to content

Change e_pin_type to enum class #3250

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 2 commits into from
Aug 25, 2025
Merged

Change e_pin_type to enum class #3250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9ab710
Change e_pin_type to enum class
AmirhosseinPoolad Aug 22, 2025
80644f4
Add comment for vpr UNDEFINED
AmirhosseinPoolad Aug 25, 2025
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
15 changes: 8 additions & 7 deletions libs/libarchfpga/src/arch_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <vector>

#include "logic_types.h"
#include "physical_types.h"
#include "vtr_assert.h"
#include "vtr_list.h"
#include "vtr_memory.h"
Expand Down Expand Up @@ -669,12 +670,12 @@ void ProcessMemoryClass(t_pb_type* mem_pb_type) {
mem_pb_type->modes[0].parent_pb_type = mem_pb_type;
mem_pb_type->modes[0].index = 0;
mem_pb_type->modes[0].mode_power = new t_mode_power();
num_pb = OPEN;
num_pb = ARCH_FPGA_UNDEFINED_VAL;
for (i = 0; i < mem_pb_type->num_ports; i++) {
if (mem_pb_type->ports[i].port_class != nullptr
&& strstr(mem_pb_type->ports[i].port_class, "data")
== mem_pb_type->ports[i].port_class) {
if (num_pb == OPEN) {
if (num_pb == ARCH_FPGA_UNDEFINED_VAL) {
num_pb = mem_pb_type->ports[i].num_pins;
} else if (num_pb != mem_pb_type->ports[i].num_pins) {
archfpga_throw(get_arch_file_name(), 0,
Expand Down Expand Up @@ -1206,7 +1207,7 @@ void setup_pin_classes(t_physical_tile_type* type) {
int num_class;

for (int i = 0; i < type->num_pins; i++) {
type->pin_class.push_back(OPEN);
type->pin_class.push_back(ARCH_FPGA_UNDEFINED_VAL);
type->is_ignored_pin.push_back(true);
type->is_pin_global.push_back(true);
}
Expand All @@ -1229,10 +1230,10 @@ void setup_pin_classes(t_physical_tile_type* type) {
class_inf.equivalence = port.equivalent;

if (port.type == IN_PORT) {
class_inf.type = RECEIVER;
class_inf.type = e_pin_type::RECEIVER;
} else {
VTR_ASSERT(port.type == OUT_PORT);
class_inf.type = DRIVER;
class_inf.type = e_pin_type::DRIVER;
}

for (int k = 0; k < port.num_pins; ++k) {
Expand Down Expand Up @@ -1263,10 +1264,10 @@ void setup_pin_classes(t_physical_tile_type* type) {
class_inf.equivalence = port.equivalent;

if (port.type == IN_PORT) {
class_inf.type = RECEIVER;
class_inf.type = e_pin_type::RECEIVER;
} else {
VTR_ASSERT(port.type == OUT_PORT);
class_inf.type = DRIVER;
class_inf.type = e_pin_type::DRIVER;
}

type->pin_class[pin_count] = num_class;
Expand Down
15 changes: 8 additions & 7 deletions libs/libarchfpga/src/physical_types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "physical_types.h"
#include "arch_types.h"
#include "vtr_math.h"
#include "vtr_util.h"
#include "vtr_log.h"
Expand Down Expand Up @@ -164,17 +165,17 @@ int t_physical_tile_type::get_sub_tile_loc_from_pin(int pin_num) const {
}
}

return OPEN;
return ARCH_FPGA_UNDEFINED_VAL;
}

bool t_physical_tile_type::is_empty() const {
return name == std::string(EMPTY_BLOCK_NAME);
}

int t_physical_tile_type::find_pin(std::string_view port_name, int pin_index_in_port) const {
int ipin = OPEN;
int ipin = ARCH_FPGA_UNDEFINED_VAL;
int port_base_ipin = 0;
int num_port_pins = OPEN;
int num_port_pins = ARCH_FPGA_UNDEFINED_VAL;
int pin_offset = 0;

bool port_found = false;
Expand All @@ -197,7 +198,7 @@ int t_physical_tile_type::find_pin(std::string_view port_name, int pin_index_in_
pin_offset += sub_tile.num_phy_pins;
}

if (num_port_pins != OPEN) {
if (num_port_pins != ARCH_FPGA_UNDEFINED_VAL) {
VTR_ASSERT(pin_index_in_port < num_port_pins);

ipin = port_base_ipin + pin_index_in_port + pin_offset;
Expand All @@ -207,14 +208,14 @@ int t_physical_tile_type::find_pin(std::string_view port_name, int pin_index_in_
}

int t_physical_tile_type::find_pin_class(std::string_view port_name, int pin_index_in_port, e_pin_type pin_type) const {
int iclass = OPEN;
int iclass = ARCH_FPGA_UNDEFINED_VAL;

int ipin = find_pin(port_name, pin_index_in_port);

if (ipin != OPEN) {
if (ipin != ARCH_FPGA_UNDEFINED_VAL) {
iclass = pin_class[ipin];

if (iclass != OPEN) {
if (iclass != ARCH_FPGA_UNDEFINED_VAL) {
VTR_ASSERT(class_inf[iclass].type == pin_type);
}
}
Expand Down
18 changes: 10 additions & 8 deletions libs/libarchfpga/src/physical_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
* Authors: Jason Luu and Kenneth Kent
*/

#include <cstdint>
#include <functional>
#include <utility>
#include <vector>
Expand All @@ -42,6 +43,7 @@
#include "logic_types.h"
#include "clock_types.h"
#include "switchblock_types.h"
#include "arch_types.h"

#include "vib_inf.h"

Expand Down Expand Up @@ -163,7 +165,7 @@ struct t_metadata_dict : vtr::flat_map<

/* Pins describe I/O into clustered logic block.
* A pin may be unconnected, driving a net or in the fanout, respectively. */
enum e_pin_type {
enum class e_pin_type : int8_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just curious, why use an int8 here instead of leaving it unspecified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the hot structs in rr-graph used e_pin_type::OPEN as default value so I thought making it smaller would make a difference. Halfway through the changes I realized it was actually not OPEN but simply -1. Reverted that change but the int8_t remained. I could remove this if think it'll be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About UNDEFINED in vpr vs the other constants in libarchfpga and librrgraph, yeah I'm also not sure. I made each project have it's own UNDEFINED which is not ideal because of the reasons you mentioned but also to do otherwise we'll need a project wide constant. If you know a good place for that constant I'll be more than happy to unify all the different -1s.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not 100% sure what the compiler will do in this case. If you leave it unspecified it will use the default int type. This is whatever bitwidth of integer that the CPU finds the most convenient (which I think will be the size of a register). When you use a smaller type (like int8_t in this case), the compiler will still have to put the enum variable in a register which will certainly be larger so it may have to add the appropriate masking to the register so it only grabs the part that represents the enum. This can be slower, but honestly its probably so little its not worth worrying about. Also I suspect compilers are smart enough to recognize this behavior.

The only benefit in my opinion on using a smaller int type for this would be if you are storing a large array of enums; but if we are not doing that, its probably cleaner just to leave as "int"; but I have seen people fight online about this (they claim that forcing the type helps protect people who are running on different machines, but I doubt that will be a problem in this case).

Honestly, up to you if you want to change it. If you are making any other changes to this PR, maybe change it back. But thats why I say it does not really matter. It is super interesting though in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding UNDEFINED, I agree. I also do not think there is a good central place for it. Ideally we would use more modern std::optional syntax when we can; but that is far too much for this PR. Do not let it block it.

OPEN = -1,
DRIVER = 0,
RECEIVER = 1
Expand Down Expand Up @@ -873,9 +875,9 @@ struct t_physical_pin {
* above the base die, the layer_num is 1 and so on.
*/
struct t_physical_tile_loc {
int x = OPEN;
int y = OPEN;
int layer_num = OPEN;
int x = ARCH_FPGA_UNDEFINED_VAL;
int y = ARCH_FPGA_UNDEFINED_VAL;
int layer_num = ARCH_FPGA_UNDEFINED_VAL;

t_physical_tile_loc() = default;

Expand All @@ -884,9 +886,9 @@ struct t_physical_tile_loc {
, y(y_val)
, layer_num(layer_num_val) {}

// Returns true if this type location layer_num/x/y is not equal to OPEN
// Returns true if this type location layer_num/x/y is not equal to ARCH_FPGA_UNDEFINED_VAL
operator bool() const {
return !(x == OPEN || y == OPEN || layer_num == OPEN);
return !(x == ARCH_FPGA_UNDEFINED_VAL || y == ARCH_FPGA_UNDEFINED_VAL || layer_num == ARCH_FPGA_UNDEFINED_VAL);
}
};

Expand Down Expand Up @@ -1369,7 +1371,7 @@ class t_pb_graph_node {
* There is a root-level pb_graph_node assigned to each logical type. Each logical type can contain multiple primitives.
* If this pb_graph_node is associated with a primitive, a unique number is assigned to it within the logical block level.
*/
int primitive_num = OPEN;
int primitive_num = ARCH_FPGA_UNDEFINED_VAL;

/* Contains a collection of mode indices that cannot be used as they produce conflicts during VPR packing stage
*
Expand Down Expand Up @@ -1578,7 +1580,7 @@ class t_pb_graph_edge {
int* pack_pattern_indices;
bool infer_pattern;

int switch_type_idx = OPEN; /* architecture switch id of the edge - used when flat_routing is enabled */
int switch_type_idx = ARCH_FPGA_UNDEFINED_VAL; /* architecture switch id of the edge - used when flat_routing is enabled */

// class member functions
public:
Expand Down
34 changes: 18 additions & 16 deletions libs/libarchfpga/src/physical_types_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#include <numeric>
#include "arch_types.h"
#include "physical_types.h"
#include "vtr_assert.h"
#include "vtr_util.h"

Expand Down Expand Up @@ -182,8 +184,8 @@ static t_pin_inst_port block_type_pin_index_to_pin_inst(t_physical_tile_type_ptr
pin_inst_port.logical_block_index = logical_num;
pin_inst_port.pb_type_idx = pb_type_idx;
pin_inst_port.pin_physical_num = pin_physical_num;
pin_inst_port.port_index = OPEN;
pin_inst_port.pin_index_in_port = OPEN;
pin_inst_port.port_index = ARCH_FPGA_UNDEFINED_VAL;
pin_inst_port.pin_index_in_port = ARCH_FPGA_UNDEFINED_VAL;

if (is_flat && logical_num != -1) {
auto pb_pin = get_pb_pin_from_pin_physical_num(type, pin_physical_num);
Expand All @@ -200,8 +202,8 @@ static t_pin_inst_port block_type_pin_index_to_pin_inst(t_physical_tile_type_ptr
}
}
}
VTR_ASSERT(pin_inst_port.port_index != OPEN);
VTR_ASSERT(pin_inst_port.pin_index_in_port != OPEN);
VTR_ASSERT(pin_inst_port.port_index != ARCH_FPGA_UNDEFINED_VAL);
VTR_ASSERT(pin_inst_port.pin_index_in_port != ARCH_FPGA_UNDEFINED_VAL);
return pin_inst_port;
}

Expand Down Expand Up @@ -435,7 +437,7 @@ int get_sub_tile_physical_pin(int sub_tile_index,

int get_logical_block_physical_sub_tile_index(t_physical_tile_type_ptr physical_tile,
t_logical_block_type_ptr logical_block) {
int sub_tile_index = OPEN;
int sub_tile_index = ARCH_FPGA_UNDEFINED_VAL;
for (const auto& sub_tile : physical_tile->sub_tiles) {
auto eq_sites = sub_tile.equivalent_sites;
auto it = std::find(eq_sites.begin(), eq_sites.end(), logical_block);
Expand All @@ -444,7 +446,7 @@ int get_logical_block_physical_sub_tile_index(t_physical_tile_type_ptr physical_
}
}

if (sub_tile_index == OPEN) {
if (sub_tile_index == ARCH_FPGA_UNDEFINED_VAL) {
archfpga_throw(__FILE__, __LINE__,
"Found no instances of logical block type '%s' within physical tile type '%s'. ",
logical_block->name.c_str(), physical_tile->name.c_str());
Expand All @@ -458,7 +460,7 @@ int get_physical_pin(t_physical_tile_type_ptr physical_tile,
int pin) {
int sub_tile_index = get_logical_block_physical_sub_tile_index(physical_tile, logical_block);

if (sub_tile_index == OPEN) {
if (sub_tile_index == ARCH_FPGA_UNDEFINED_VAL) {
archfpga_throw(__FILE__, __LINE__,
"Couldn't find the corresponding physical tile type pin of the logical block type pin %d.",
pin);
Expand All @@ -471,7 +473,7 @@ int get_physical_pin(t_physical_tile_type_ptr physical_tile,
int get_logical_block_physical_sub_tile_index(t_physical_tile_type_ptr physical_tile,
t_logical_block_type_ptr logical_block,
int sub_tile_capacity) {
int sub_tile_index = OPEN;
int sub_tile_index = ARCH_FPGA_UNDEFINED_VAL;
for (const auto& sub_tile : physical_tile->sub_tiles) {
auto eq_sites = sub_tile.equivalent_sites;
auto it = std::find(eq_sites.begin(), eq_sites.end(), logical_block);
Expand All @@ -482,7 +484,7 @@ int get_logical_block_physical_sub_tile_index(t_physical_tile_type_ptr physical_
}
}

if (sub_tile_index == OPEN) {
if (sub_tile_index == ARCH_FPGA_UNDEFINED_VAL) {
archfpga_throw(__FILE__, __LINE__,
"Found no instances of logical block type '%s' within physical tile type '%s'. ",
logical_block->name.c_str(), physical_tile->name.c_str());
Expand Down Expand Up @@ -528,7 +530,7 @@ int get_physical_pin_at_sub_tile_location(t_physical_tile_type_ptr physical_tile
VTR_ASSERT(pin < physical_tile->num_pins);
int sub_tile_index = get_logical_block_physical_sub_tile_index(physical_tile, logical_block, sub_tile_capacity);

if (sub_tile_index == OPEN) {
if (sub_tile_index == ARCH_FPGA_UNDEFINED_VAL) {
archfpga_throw(__FILE__, __LINE__,
"Couldn't find the corresponding physical tile type pin of the logical block type pin %d.",
pin);
Expand Down Expand Up @@ -610,7 +612,7 @@ bool is_opin(int ipin, t_physical_tile_type_ptr type) {

int iclass = type->pin_class[ipin];

if (type->class_inf[iclass].type == DRIVER)
if (type->class_inf[iclass].type == e_pin_type::DRIVER)
return true;
else
return false;
Expand Down Expand Up @@ -895,7 +897,7 @@ int get_tile_class_max_ptc(t_physical_tile_type_ptr tile, bool is_flat) {
/** get information given pin physical number **/
std::tuple<const t_sub_tile*, int> get_sub_tile_from_pin_physical_num(t_physical_tile_type_ptr physical_tile, int physical_num) {
const t_sub_tile* target_sub_tile = nullptr;
int target_sub_tile_cap = OPEN;
int target_sub_tile_cap = ARCH_FPGA_UNDEFINED_VAL;

bool pin_on_tile = is_pin_on_tile(physical_tile, physical_num);

Expand Down Expand Up @@ -926,7 +928,7 @@ t_logical_block_type_ptr get_logical_block_from_pin_physical_num(t_physical_tile
t_logical_block_type_ptr logical_block = nullptr;

std::tie(sub_tile, sub_tile_cap) = get_sub_tile_from_pin_physical_num(physical_tile, physical_num);
VTR_ASSERT(sub_tile_cap != OPEN);
VTR_ASSERT(sub_tile_cap != ARCH_FPGA_UNDEFINED_VAL);

for (auto logical_block_pin_range_pair : sub_tile->intra_pin_range[sub_tile_cap]) {
if (physical_num >= logical_block_pin_range_pair.second.low) {
Expand Down Expand Up @@ -1189,7 +1191,7 @@ int get_pb_pin_physical_num(t_physical_tile_type_ptr physical_tile,
t_logical_block_type_ptr logical_block,
int relative_cap,
const t_pb_graph_pin* pin) {
int pin_physical_num = OPEN;
int pin_physical_num = ARCH_FPGA_UNDEFINED_VAL;
if (pin->is_root_block_pin()) {
pin_physical_num = get_physical_pin_at_sub_tile_location(physical_tile,
logical_block,
Expand Down Expand Up @@ -1264,12 +1266,12 @@ bool intra_tile_nodes_connected(t_physical_tile_type_ptr physical_type,
const t_sub_tile* from_sub_tile;
int from_sub_tile_rel_cap;
std::tie(from_sub_tile, from_sub_tile_rel_cap) = get_sub_tile_from_pin_physical_num(physical_type, pin_physical_num);
VTR_ASSERT(from_sub_tile != nullptr && from_sub_tile_rel_cap != OPEN);
VTR_ASSERT(from_sub_tile != nullptr && from_sub_tile_rel_cap != ARCH_FPGA_UNDEFINED_VAL);

const t_sub_tile* to_sub_tile;
int to_sub_tile_rel_cap;
std::tie(to_sub_tile, to_sub_tile_rel_cap) = get_sub_tile_from_class_physical_num(physical_type, sink_physical_num);
VTR_ASSERT(to_sub_tile != nullptr && to_sub_tile_rel_cap != OPEN);
VTR_ASSERT(to_sub_tile != nullptr && to_sub_tile_rel_cap != ARCH_FPGA_UNDEFINED_VAL);

return (from_sub_tile_rel_cap == to_sub_tile_rel_cap) && (from_sub_tile == to_sub_tile);

Expand Down
11 changes: 6 additions & 5 deletions libs/libarchfpga/src/read_fpga_interchange_arch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

#include "read_fpga_interchange_arch.h"
#include "arch_types.h"

#ifdef VTR_ENABLE_CAPNPROTO

Expand Down Expand Up @@ -550,7 +551,7 @@ struct ArchReader {
std::string wire_name = str(wire.getName());

// pin name, bel name
int pin_id = OPEN;
int pin_id = ARCH_FPGA_UNDEFINED_VAL;
bool pad_exists = false;
bool all_inout_pins = true;
std::string pad_bel_name;
Expand All @@ -577,7 +578,7 @@ struct ArchReader {
pin_id = pin;
}

if (pin_id == OPEN) {
if (pin_id == ARCH_FPGA_UNDEFINED_VAL) {
// If no driver pin has been found, the assumption is that
// there must be a PAD with inout pin connected to other inout pins
for (auto pin : wire.getPins()) {
Expand All @@ -594,7 +595,7 @@ struct ArchReader {
}
}

VTR_ASSERT(pin_id != OPEN);
VTR_ASSERT(pin_id != ARCH_FPGA_UNDEFINED_VAL);

auto out_pin = site.getBelPins()[pin_id];
auto out_pin_bel = get_bel_reader(site, str(out_pin.getBel()));
Expand Down Expand Up @@ -1674,10 +1675,10 @@ struct ArchReader {
* If a bel name index is specified, the bel pins are processed, otherwise the site ports
* are processed instead.
*/
void process_block_ports(t_pb_type* pb_type, Device::SiteType::Reader& site, size_t bel_name = OPEN) {
void process_block_ports(t_pb_type* pb_type, Device::SiteType::Reader& site, size_t bel_name = ARCH_FPGA_UNDEFINED_VAL) {
// Prepare data based on pb_type level
std::set<std::tuple<std::string, PORTS, int>> pins;
if (bel_name == (size_t)OPEN) {
if (bel_name == (size_t)ARCH_FPGA_UNDEFINED_VAL) {
for (auto pin : site.getPins()) {
auto dir = pin.getDir() == INPUT ? IN_PORT : OUT_PORT;
pins.emplace(str(pin.getName()), dir, 1);
Expand Down
Loading