-
Notifications
You must be signed in to change notification settings - Fork 448
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
DPDK backend: Add support for tuple expressions in transition select statement #2836
Conversation
1) Add missing regarray declarations 2) Fix read() method implementation
…n objects and methods in/from control blocks only
- Disabled Midend predication pass to allow conditional operator to be splitted into if-else statement - Added a new test and reference output for conditional operator
…830/p4c into ushag_conditional_operator
…ement. Fixed few typos
Why don't you first use the SingleArgumentSelect midend pass in dpdk midend.cpp and then change the dpdk backend like #2827 has done? |
If dpdk is limited to 64 bits per value this implementation is more general |
backends/dpdk/dpdkProgram.cpp
Outdated
auto left = maskexpr->left; | ||
auto right = maskexpr->right; | ||
/* Dpdk architecture requires that both operands of &&& be constants */ | ||
if (!left->is<IR::Constant>() || !left->is<IR::Constant>()) { |
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.
You are using the same expression twice.
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 for catching this. When I tried hitting this error with a testcase, I see this condition is already checked by midend in DoSimplifySelectCases pass. So will remove this error check altogether.
I don't see any benefit in using the SingleArgumentSelect pass as there is no corresponding assembly instruction for concat operator and we will have to ultimately convert the case value into series of jmpneqs. |
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 will approve, but please consider using a name generator. Maybe you want to file an issue about it.
backends/dpdk/dpdkProgram.cpp
Outdated
void ConvertToDpdkParser::getCondVars(const IR::Expression *sv, const IR::Expression *ce, | ||
IR::Expression **leftExpr, IR::Expression **rightExpr) { | ||
if (sv->is<IR::Constant>() && sv->type->width_bits() > 32) { | ||
::error(ErrorType::ERR_UNEXPECTED, |
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.
This is a back end constraint so perhaps you should use one of the unsupported errors.
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.
Updated
backends/dpdk/dpdkProgram.cpp
Outdated
/* For each select case, emit the block start label and then a series of | ||
jmp instructions. */ | ||
for (auto sc : caseList) { | ||
cstring labelName = state->name + "_" + std::to_string(count++); |
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.
This looks like it could generate collisions if some states already have such names. I would always recommend using a name generator, but you have to use it everywhere so it keeps track of all used symbols.
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 have updated the code to use refmap->newName() for generating the case label.
@mbudiu-vmw I have updated the code as per your comments. If there are no further comments, could you please merge this PR? |
…statement (p4lang#2836) * Fix dpdk regression failure: Duplicates declaration * Fix cpplint errors * Added checks to ensure parameters exist before accessing them * Fix PSA extern Register's implementation 1) Add missing regarray declarations 2) Fix read() method implementation * Updated spec.cpp as per review comments * Emit a warning if Register instantiation is found outside control block * Fixed a typo * Remove restriction for instantiation and invocation of register extern objects and methods in/from control blocks only * Conditional operator support for P4C-DPDK. - Disabled Midend predication pass to allow conditional operator to be splitted into if-else statement - Added a new test and reference output for conditional operator * Changing ternary match to exact in test program, updating the reference output * Added support for range and mask operations in transition select statement. Fixed few typos * Support mask operations in dpdk backend (#1) * Remove pass for inserting temporary mask variables * update reference output * Address review comments * DPDK backend: Add support for tuple expressions in transition select statement * Address review comments for tuple expression support * Use refmap to create label names, use ERR_UNSUPPORTED_ON_TARGET for a few errors * Update test files to include core.p4
This PR adds support for the tuple expressions in transition select statement.
The changes include:
New function for handling the tuple expressions.
New helpers functions to refactor some of existing code.
Added new tests and reference outputs
The tuple expressions are evaluated as exact match of each element in the tuple and is
translated to dpdk assembly by short-circuiting the conditions where beginning of each keyset
starts with a unique case label.