-
Notifications
You must be signed in to change notification settings - Fork 442
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
Support for 128 bit bitwise operation #4952
base: main
Are you sure you want to change the base?
Conversation
Sosutha
commented
Oct 9, 2024
- Added movh statement definition
- Added support for 128 bitwise operation
- Updated testcase and output
9e9f473
to
a0e34e4
Compare
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.
Please check the comments
backends/dpdk/dpdkArch.h
Outdated
} | ||
} | ||
} | ||
return false; |
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.
Is it valid input to have operand1 of 128 bits and operand2 less than 128 bits at this stage?
If true, isValidOperandSize in line number 1106 may issue error for operand being greater than 64 bits.
IMO, isValidOperandSize function can be parameterized to check limit of 128 bits in exceptional cases.
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.
Only operations where both operands are of size 128 bits are being supported for now.
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.
Typechecking will ensure that the operands to any binary operation like these are the exact same type, so there's really no need to check both. The only binops that can have different types for their operands are shifts, array index, and concat. If you want to be paranoid, you can BUG_CHECK(src1Op->type == src2Op->type, ...
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 the code.
backends/dpdk/dpdkHelpers.cpp
Outdated
fields->push_back(new IR::StructField("tmp", IR::Type_Bits::get(64))); | ||
const IR::Type_Header *headerStruct = new IR::Type_Header(IR::ID("tmp128_t"), *fields); | ||
auto name = new cstring("tmp128_t"); | ||
structure->header_types.emplace(*name, headerStruct); |
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.
structure->header_types.emplace(*name, headerStruct); | |
structure->header_types.emplace(cstring("tmp128_t"), headerStruct); |
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 the code
e20762a
to
30b7c86
Compare
Suggest to try to compile DASH code as a test of completeness to see if the code compiles/builds? If DASH has other than bitwise operations, it may fail. Do not support additions to IPv6 addresses (we would likely not do this), but all else s/be supported. Can we add @jafingerhut as a reviewer please? |
015e0a9
to
80518e8
Compare
Updated on 2024-Oct-10 to correct the p4c-dpdk command line options. I still get errors saying 'Unupported bitwidth 128' in them with the updated command line. I tried checking out this modified branch of the code, and built
I see many warnings, which are unrelated to these changes, but I also see 7 errors "Unsupported bitwidth 128" as copied below:
Do you see these as well, Sosutha? It would be good to understand why these errors occur, and if possible, either modify this PR so that those errors do not occur, or figure out a modified version of the DASH program that behaves as it is written today, but does not give those 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.
This PR should not be merged until the compilator for https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples/dash/dash-pipeline-pna-dpdk.p4 passes
0b866d7
to
ae74cb8
Compare
Andy, I am getting same errors as you. This PR addresses only 128 bitwise operations. I can add implementation for comparison operations to this same PR. So errors related to that will be solved with that implementation. For error in apply statement, I will have to check and revert back |
We can also add the failing compilation to XFails for now. The important part is that this program is added as a test that is checked. |
7660938
to
ad8864c
Compare
Signed-off-by: Sosutha Sethuramapandian <[email protected]> * Added movh statement definition * Added support for 128 bitwise operation * Added support for 128 bit equal and not equal operators. * Updated testcase and output
ad8864c
to
6a6a63e
Compare