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

DPDK Backend: Fix binary operations with 1st operand is not same as dst operand #3209

Merged
merged 3 commits into from
Apr 14, 2022

Conversation

usha1830
Copy link
Contributor

@usha1830 usha1830 commented Apr 12, 2022

DPDK needs the binary operation to have 1st src operand same as dst operand.
Some cases with non commutative operations were not correctly handled by the existing transformation pass.
For example:
a = b << a
should be converted to

tmp =b
tmp = tmp << a
a = tmp

@hanw
Copy link
Contributor

hanw commented Apr 13, 2022

You can save one more instruction with

a = a - 10
a = -a

@usha1830
Copy link
Contributor Author

You can save one more instruction with

a = a - 10
a = -a

Han, the transformation also applies to other non-commutative operations like shift
I have updated the PR description to avoid confusion, will also update the code comment for this.

@hanw
Copy link
Contributor

hanw commented Apr 13, 2022

Ok, but my comment is still valid.

return false;
}

bool isStandardMetadata(cstring name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is actually PSA-specific, not DPDK specific. So it should probably move to some common file eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about creating a utils.h/cpp under backends/common/ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably move more stuff in such a place. But it should be in some directory named "psa".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specific function is for both psa and pna metadata. Of course, we can split it up if we decide to go with arch-specific common utils.
Can this PR be merged if this does not need to be an immediate action?

@usha1830
Copy link
Contributor Author

Ok, but my comment is still valid.

Yes. Do you think, this case should be specially handled?

Also, I don't see any instruction for negation here
https://doc.dpdk.org/api/rte__swx__pipeline_8h.html#acf7715fccb903f8eca8b000953b188cb

@usha1830 usha1830 changed the title DPDK Backend: Fix binary operations with 1st source operand a constant DPDK Backend: Fix binary operations with 1st operand is not same as dst operand Apr 14, 2022
@hanw
Copy link
Contributor

hanw commented Apr 14, 2022

Ok, but my comment is still valid.

Yes. Do you think, this case should be specially handled?

Also, I don't see any instruction for negation here https://doc.dpdk.org/api/rte__swx__pipeline_8h.html#acf7715fccb903f8eca8b000953b188cb

Thanks for the pointer. This looks fine.

@mihaibudiu mihaibudiu merged commit 262ebe4 into p4lang:main Apr 14, 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