Change e_pin_type to enum class #3250
Conversation
github-actions
bot
added
VPR
There was a problem hiding this comment.
LGTM, this is a very very welcome change.
I am not the biggest fan of using "UNDEFINED" everywhere in the VPR side since it may get compared with the RR_graph and the libarchfpga UNDEFINES and cause potential issues if they did not happen to be equal; however, what it was before was much worse.
I just left one question about using an int8 for the enum instead of using the default value. It does not really matter but I was just curious if you found a performance / memory change.
| @@ -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 { | |||
There was a problem hiding this comment.
I am just curious, why use an int8 here instead of leaving it unspecified?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| #define UNDEFINED (-1) | ||
| #endif | ||
|
|
||
| constexpr int UNDEFINED = -1; |
There was a problem hiding this comment.
Should comment this line. Used for illegal / undefined values of indices, where legal ones should always be 0 or greater.
AmirhosseinPoolad
force-pushed
the
archfpga_enum_class
branch
from
dd831c4 to
d9ab710
Compare
|
LGTM, feel free to merge whenever CI passes. |
Changed e_pin_type to enum class. Since e_pin_type::OPEN was widely used throughout the codebase as a generic "undefined" constant, change most of the e_pin_type::OPEN values to a corresponding constexpr value. As a guideline, most e_pin_type::OPEN values that were assigned to ints or were an index of some kind and in general weren't related to pin types were changed to something else, but it is very possible that I mistakenly changed something that should've been a e_pin_type to something else.
Please verify that any e_pin_type::OPEN that was changed to a constexpr with a value of -1 wasn't actually an e_pin_type.