-
Notifications
You must be signed in to change notification settings - Fork 13
Add initial implementation for BMv2 headers and parsers #272
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.
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
base: main
Are you sure you want to change the base?
Conversation
de8c486 to
982a88c
Compare
asl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See first round of comments
| LogicalResult matchAndRewrite(P4CoreLib::PacketExtractOp op, OpAdaptor operands, | ||
| ConversionPatternRewriter &rewriter) const override { | ||
| auto context = op.getContext(); | ||
| auto fieldRefOp = op.getHdr().getDefiningOp<P4HIR::StructFieldRefOp>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we cannot extract to arbitrary variable? E.g.
bit<32> foo;
packet_in.extract(foo);
?
How does BMV2 backend deal with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the particular example in your comment, p4c raises an error if I try to pass a bit<42> variable to extract (says only headers are allowed). If we have an header instance as a local variable, the header instance will be part of the headers JSON node, and the extract node can refer to it from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, then we'd need similar error here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what's about the error here? Here is the place where we'd need to emit user-visible errors in case of match failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed the comment. I've added the check for header type, although I'm not sure here it's the right place for this type of errors, maybe it's better to raise this in the front end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is a target-specific limitation. So it should be handled during the translation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense, we got the error message now 👍
| LogicalResult matchAndRewrite(P4HIR::ParserOp op, OpAdaptor operands, | ||
| ConversionPatternRewriter &rewriter) const override { | ||
| auto loc = op.getLoc(); | ||
| auto firstTransition = cast<P4HIR::ParserTransitionOp>(op.getBody().back().getTerminator()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getStartState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think I still need op.getBody().back().getTerminator() to access the actual transition op since we want to remove it from the region after retrieving the starting state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add then something like getStartTransition to the ParserOp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you
982a88c to
8ee02fd
Compare
7a5bbfb to
c944090
Compare
Signed-off-by: Pietro Ghiglio <[email protected]>
27435ee to
bc10900
Compare
This PR adds an initial implementation for BMv2 Headers and Parser. Only a few parsing constructs are supported, more will be added with follow up PRs.
In order to match the BMv2 json spec, transitions, transition keys and parsing operations are split into three separate regions (with appropriate checks in the verifier) that resemble the final json nodes that we want to emit.