Add new methods to IR types to support varbit #5460
Conversation
vbnogueira
added
the
p4tc
vbnogueira
force-pushed
the
varbit_support_ir_part
branch
from
95eef58 to
dbc1283
Compare
ChrisDodd
left a comment
ChrisDodd
left a comment
There was a problem hiding this comment.
This is not quite the same as the code in TypeMap::widthBits (in frontends/p4/typeMap.cpp). It should be possible to simplify that code and make it more consistent by calling this code.
ir/type.def
Outdated
|
|
||
| auto first = fields.begin(); | ||
| int rv = (*first)->type->min_or_fixed_width_bits(); | ||
| for (auto f = fields.begin() + 1; f != fields.end(); f++) |
There was a problem hiding this comment.
could use
int rv = INT_MAX;
for (auto f : fields) rv = std::min(rv, f->type->min_or_fixed_width_bits());
| if (f->type->variable()) return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Do we need an overload of Type_HeaderUnion::variable() which returns true if there are fields of different size?
There was a problem hiding this comment.
I might be missing something, but I don't think so
This is mostly to tell whether there is a field in the header union which has variable length
There was a problem hiding this comment.
Is "a field which has variable length" or "a field of type varbit" -- the language defines 3 type constructs that have a variable length -- varbit, header unions, and header stacks. header unions are weird in that if all the alternatives have the same size, they are not really variable length.
One could even claim that all headers are "variable length" as they maybe valid or invalid, and when invalid, they have a length of 0. That is not too useful for most purposes. Generally what one wants to know is the maximum length, which is what the basic width_bits returns.
ir/type.def
Outdated
| } | ||
| int min_width_bits() const override { | ||
| int rv; | ||
| rv = 0; |
| int rv = 0; | ||
| for (auto f : components) { | ||
| rv += f->max_or_fixed_width_bits(); | ||
| } |
There was a problem hiding this comment.
We're inconsistent as to whether we use {..} in a for loop when the body is a single line. We should pick and be consistent. IMO it is better to NOT use them, as the "mostly-blank" lines with just a } break up the visual flow and make it harder to see structure when scanning the code.
While fixing all the code to be consistent is not the purpose of this change, it should at least be consistent in what it adds -- either add them around lines 513 and 519 or remove them here and elsewhere.
There was a problem hiding this comment.
I ended up removing {..} from the loops
There was a problem hiding this comment.
We should pick and be consistent
I disagree with single-line without brackets if/for. They are often easy to miss when reading code. It also allows for if/for without brackets in general which often leads to bugs.
https://clang.llvm.org/extra/clang-tidy/checks/readability/braces-around-statements.html is a good check
There was a problem hiding this comment.
Okk, will add the brackets
There was a problem hiding this comment.
I disagree -- that link is a perfect example of why it is bad -- it makes the code much harder to read in order to possibly reduce future bugs from programmers adding extra lines and not added the brackets (only) when they are needed.
There was a problem hiding this comment.
@fruffy what do you think?
I can put the brackets back if needed
vbnogueira
force-pushed
the
varbit_support_ir_part
branch
from
dbc1283 to
c17a4dc
Compare
I see, I can try to take a look at how complex it would be for |
|
Looking at your other PR, it seems that various |
The |
Add the following new methods: - min_width_bits: Determines the minumum type's bit size - max_width_bits: Determines the maximum type's bit size - min_or_fixed_width_bits: Same as min_width_bits but also works for non-variable types - max_or_fixed_width_bits: Same as max_width_bits but also works for non-variable types - variable: Tells whether the type has variable length Signed-off-by: Mouse <[email protected]>
vbnogueira
force-pushed
the
varbit_support_ir_part
branch
from
c17a4dc to
db7a8d0
Compare
4 participants
Add the following new methods: