Compiler Bug: overwriting definitions in defuse analysis #2900
Assignees
Comments
|
To reproduce: |
|
I can confirm that p4test also gives a similar compiler bug for this program, so it is not unique to the p4c-dpdk back end. The compiler bug disappears if you make any of the following small changes to the test program:
There seems to be something in the p4c front end code (or I suppose it could be something common between the DPDK and p4test back ends) that does not work with the combination of a table applied multiple times, and a key expression that calls a function. The Compiler bug occurs regardless of whether that function is one defined in the P4 program itself, or an extern function. |
mvido
added a commit
that referenced
this issue
Nov 30, 2021
This commit fixes bug: #2900 Frontend pass P4::KeySideEffect called by P4::SideEffectOrdering moves key fields computations prior to the table application when key computation involves side effects which happens f.e. when there is a function call (as in the sample program which demonstrates the bug). In our sample program MainControlImpl: control MainControlImpl( inout headers_t hdr, inout main_metadata_t meta, in pna_main_input_metadata_t istd, inout pna_main_output_metadata_t ostd) { table clb_pinned_flows { key = { // other key fields also possible, e.g. VRF SelectByDirection(istd.direction, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr): exact @name("ipv4_addr_0"); SelectByDirection(istd.direction, hdr.ipv4.dstAddr, hdr.ipv4.srcAddr): exact @name("ipv4_addr_1"); hdr.ipv4.protocol : exact; } actions = { NoAction; } const default_action = NoAction; } apply { if (TxPkt(istd) && pass_1st(istd)) { clb_pinned_flows.apply(); } else if (TxPkt(istd) && pass_2nd(istd)) { clb_pinned_flows.apply(); } } } is transformed to: control MainControlImpl(inout headers_t hdr, inout main_metadata_t meta, in pna_main_input_metadata_t istd, inout pna_main_output_metadata_t ostd) { bit<32> key_0; bit<32> key_1; bit<8> key_2; @name("clb_pinned_flows") table clb_pinned_flows_0 { key = { key_0: exact @name("ipv4_addr_0") ; key_1: exact @name("ipv4_addr_1") ; key_2: exact @name("hdr.ipv4.protocol") ; } actions = { NoAction(); } const default_action = NoAction(); } bool tmp; bool tmp_0; bool tmp_1; bool tmp_2; bool tmp_3; bool tmp_4; apply { { tmp = TxPkt(istd); if (!tmp) { tmp_0 = false; } else { tmp_1 = pass_1st(istd); tmp_0 = tmp_1; } if (tmp_0) { key_0 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr); key_1 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.dstAddr, hdr.ipv4.srcAddr); key_2 = hdr.ipv4.protocol; clb_pinned_flows_0.apply(); } else { tmp_2 = TxPkt(istd); if (!tmp_2) { tmp_3 = false; } else { tmp_4 = pass_2nd(istd); tmp_3 = tmp_4; } if (tmp_3) { key_0 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr); key_1 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.dstAddr, hdr.ipv4.srcAddr); key_2 = hdr.ipv4.protocol; clb_pinned_flows_0.apply(); } } } } } We can see that the same assignments are before each table apply() method call. In fact, in the IR, the same assignment before each apply() method call is represented by identical IR::AssignmentStatement. Then in P4::AllDefinitions::setDefinitionsAt() when we check, if we do not overwrite the definitions for a P4::ProgramPoint for the second assignment statement, the P4::ProgramPoint is equal to the P4::ProgramPoint of the first assignment statement and thus the BUG_CHECK fails. The proposed solution is to change P4::KeySideEffect pass to create equivalent but not identical IR::AssignmentStatement(s) before each table apply() method call instead of identical IR::AssignmentStatement(s) using the clone() method.
Jovana-Syrmia
added a commit
to Jovana-Syrmia/p4c
that referenced
this issue
Dec 1, 2021
* Bug in file def_use in method setDefinitionsAt class AllDefinitions * In that method definition in map atPoint is overridden * This happens beacuse there is more than one AsssignStatement with the same id * Pass KeySideEffect is creating those AssignStatements (in method KeySideEffect::doStatement) * This method creates a BlockStatement and inserts AssignStatements from TableInsertions variable and then inserts that block before tbl.apply() * Bug is removed by cloning an AssignStatement before inserting it in a BlockStatement * Backend pass CopyMatchKeysToSingleStruct inherits KeySideEffect * Backend uses this pass to transform tables such that all the Match keys are part of the same header/metadata struct * With cloning two test fail (psa-dpdk-table-key-consolidation-if*) in spec file * This happens because for every assignment DpdkMovStatement is created and inserted in instractions (pass ConvertStatementToDpdk) * There is no need for this new DpdkMovStatements because src is already in dest (it is done the first time we call tbl.apply) * This is fixed by setting SideEffect::doStatement to be virtual and overriding it in class CopyMatchKeysToSingleStruct * In this override there is no cloning * Added test for issue 2900 (bug.p4 that was given in issue discussion) * Added expected outputs for that test
mihaibudiu
pushed a commit
that referenced
this issue
Dec 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The attached example pna program triggers a bug check in frontend defuse analysis pass.
The error message is
bug.p4.txt
The text was updated successfully, but these errors were encountered: