Skip to content
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

P4C-DPDK - Incorrect constant value when there is a substract operation involved with a constant #3411

Merged
merged 3 commits into from
Jul 20, 2022

Conversation

github-sajan
Copy link
Contributor

This PR addresses the issue in dpdk backend when there is subtraction operation with constant.

If isSigned flag is set and is not 8-bit aligned, sign extension (32/64 bit) is done for the constant
If isSigned flag is not set and is not 8-bit aligned,
insert an "BAnd" node after the "Add" for masking the value to the original bitwidth.

32 bit or 64 bit if the bit-width of constant is greater than 32 bit
*/

if (!isEightBitAligned(src2Op) && src2Op->is<IR::Constant>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't sign-extension be done for non-constant values too? Otherwise I think that overflow handling will be incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added shift operation for sign extension for non-constant value

@github-sajan github-sajan force-pushed the subtraction_issue branch 2 times, most recently from c5cecd6 to b89f24d Compare July 4, 2022 04:25
@github-sajan github-sajan requested a review from mihaibudiu July 4, 2022 05:48
if (!isEightBitAligned(left) && !isSignExtended) {
if (right->is<IR::Add>() || right->is<IR::Sub>() ||
right->is<IR::Shl>() || right->is<IR::Shr>()) {
auto width = left->type->width_bits();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you want to use the Constant class, this may overflow in C++.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I did not get the comment. Could you please add more to understand?

backends/dpdk/dpdkHelpers.cpp Outdated Show resolved Hide resolved
if (right->is<IR::Add>() || right->is<IR::Sub>() ||
right->is<IR::Shl>() || right->is<IR::Shr>()) {
auto width = left->type->width_bits();
auto mask = (1 << width) - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

1 << width may overflow in C++, but if you use the constant class to compute it, it won't.
Constant(1) << width.

@mihaibudiu
Copy link
Contributor

Looks like several dpdk tests are failing with this change.

@github-sajan
Copy link
Contributor Author

Looks like several dpdk tests are failing with this change.

Updated the outputs

@mihaibudiu mihaibudiu merged commit a6a5b70 into p4lang:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants