p4lang
Map written `LocationSet`s to program locations (`loc_t`) instead of `IR::Expression*`s
Adds a modified version of loc_t from the midend def_use pass to the frontend one. Please see discussions in #4500, #4507, #4548 for more context. I did not add any test cases as @mihaibudiu's PRs #4539 and #4727 mask the root problem, but I did verify that these changes also fix the test cases from the below issues with an older version of the compiler that did not have Mihai's fixes yet.
Fixes #4500 Fixes #4507 Fixes #4548
The way to test this is to remove the passes that were introduced to tree-ify the DAG before def-use (e.g., Cloner in simplifyDefUse.h). Unfortunately not all of them can be removed from the compiler, since other passes may require trees as well.
This causes some tests to fail. I will investigate this later.
The way to test this is to remove the passes that were introduced to tree-ify the DAG before def-use (e.g., Cloner in simplifyDefUse.h). Unfortunately not all of them can be removed from the compiler, since other passes may require trees as well.
This causes some tests to fail. I will investigate this later.
@mihaibudiu This doesn't work because of a problem I mentioned earlier in https://github.com/p4lang/p4c/issues/4548#issuecomment-2180740655. If you disable the Cloner and RemoveHidden passes in my branch and build test testdata/p4_16_samples/issue3650.p4, you'll notice that we have a SwitchStatement that has multiple equal SwitchCase children, so the ProgramPoints for each child SwitchCase's statement are considered to be equal, and we'll encounter the "Overwriting definitions" assertion. To fix this we can consider creating a unique id for each child that belongs to a given parent IR::Node during traversal, and use this additional piece of info to help uniquely identify each ProgramPoint and loc_t. This is a slightly different problem so I'd prefer that we fix it in a different future PR instead of this one.
Disabling Cloner and RemoveHidden on this branch causes the following tests to fail:
300 - p4/testdata/p4_16_samples/header-stack-ops-bmv2.p4 (Failed)
355 - p4/testdata/p4_16_samples/issue1127-bmv2.p4 (Failed)
650 - p4/testdata/p4_16_samples/issue3650.p4 (Failed)
660 - p4/testdata/p4_16_samples/issue3884.p4 (Failed)
1229 - p4/testdata/p4_16_samples/dash/dash-pipeline-pna-dpdk.p4 (Failed)
1230 - p4/testdata/p4_16_samples/dash/dash-pipeline-v1model-bmv2.p4 (Failed)
and on main branch, it causes the following tests to fail:
9 - p4/testdata/p4_16_samples/psa-example-parser-checksum.p4 (Failed)
10 - p4/testdata/p4_16_samples/action-bind.p4 (Failed)
44 - p4/testdata/p4_16_samples/arith2-inline-bmv2.p4 (Failed)
103 - p4/testdata/p4_16_samples/control-hs-index-test5.p4 (Failed)
132 - p4/testdata/p4_16_samples/drop-bmv2.p4 (Failed)
141 - p4/testdata/p4_16_samples/enum-bmv2.p4 (Failed)
187 - p4/testdata/p4_16_samples/gauntlet_action_mux-bmv2.p4 (Failed)
188 - p4/testdata/p4_16_samples/gauntlet_action_return-bmv2.p4 (Failed)
205 - p4/testdata/p4_16_samples/gauntlet_exit_combination_18-bmv2.p4 (Failed)
212 - p4/testdata/p4_16_samples/gauntlet_exit_combination_3-bmv2.p4 (Failed)
223 - p4/testdata/p4_16_samples/gauntlet_function_return-bmv2.p4 (Failed)
227 - p4/testdata/p4_16_samples/gauntlet_hdr_function_cast-bmv2.p4 (Failed)
235 - p4/testdata/p4_16_samples/gauntlet_index_4-bmv2.p4 (Failed)
236 - p4/testdata/p4_16_samples/gauntlet_index_5-bmv2.p4 (Failed)
237 - p4/testdata/p4_16_samples/gauntlet_index_6-bmv2.p4 (Failed)
238 - p4/testdata/p4_16_samples/gauntlet_index_7-bmv2.p4 (Failed)
241 - p4/testdata/p4_16_samples/gauntlet_indirect_hdr_assign_1-bmv2.p4 (Failed)
255 - p4/testdata/p4_16_samples/gauntlet_mux_validity-bmv2.p4 (Failed)
258 - p4/testdata/p4_16_samples/gauntlet_nested_switch-bmv2.p4 (Failed)
272 - p4/testdata/p4_16_samples/gauntlet_side_effect_order_4-bmv2.p4 (Failed)
292 - p4/testdata/p4_16_samples/hash-extern-bmv2.p4 (Failed)
298 - p4/testdata/p4_16_samples/header-bmv2.p4 (Failed)
300 - p4/testdata/p4_16_samples/header-stack-ops-bmv2.p4 (Failed)
324 - p4/testdata/p4_16_samples/internet_checksum1-bmv2.p4 (Failed)
327 - p4/testdata/p4_16_samples/invalid-hdr-warnings2.p4 (Failed)
355 - p4/testdata/p4_16_samples/issue1127-bmv2.p4 (Failed)
378 - p4/testdata/p4_16_samples/issue1470-bmv2.p4 (Failed)
384 - p4/testdata/p4_16_samples/issue1538.p4 (Failed)
387 - p4/testdata/p4_16_samples/issue1544-1-bmv2.p4 (Failed)
388 - p4/testdata/p4_16_samples/issue1544-2-bmv2.p4 (Failed)
390 - p4/testdata/p4_16_samples/issue1544-bmv2.p4 (Failed)
392 - p4/testdata/p4_16_samples/issue1566-bmv2.p4 (Failed)
393 - p4/testdata/p4_16_samples/issue1566.p4 (Failed)
408 - p4/testdata/p4_16_samples/issue1739-bmv2.p4 (Failed)
436 - p4/testdata/p4_16_samples/issue1955.p4 (Failed)
449 - p4/testdata/p4_16_samples/issue2104-1.p4 (Failed)
468 - p4/testdata/p4_16_samples/issue2175.p4 (Failed)
472 - p4/testdata/p4_16_samples/issue2205-1-bmv2.p4 (Failed)
478 - p4/testdata/p4_16_samples/issue2221-bmv2.p4 (Failed)
479 - p4/testdata/p4_16_samples/issue2225-bmv2.p4 (Failed)
495 - p4/testdata/p4_16_samples/issue2287-bmv2.p4 (Failed)
497 - p4/testdata/p4_16_samples/issue2288-2.p4 (Failed)
498 - p4/testdata/p4_16_samples/issue2288.p4 (Failed)
502 - p4/testdata/p4_16_samples/issue2314.p4 (Failed)
504 - p4/testdata/p4_16_samples/issue2321.p4 (Failed)
505 - p4/testdata/p4_16_samples/issue2330-1.p4 (Failed)
506 - p4/testdata/p4_16_samples/issue2330.p4 (Failed)
509 - p4/testdata/p4_16_samples/issue2343-bmv2.p4 (Failed)
512 - p4/testdata/p4_16_samples/issue2345-2.p4 (Failed)
515 - p4/testdata/p4_16_samples/issue2345.p4 (Failed)
518 - p4/testdata/p4_16_samples/issue2359.p4 (Failed)
534 - p4/testdata/p4_16_samples/issue2488-bmv2.p4 (Failed)
578 - p4/testdata/p4_16_samples/issue2844-enum.p4 (Failed)
593 - p4/testdata/p4_16_samples/issue304.p4 (Failed)
650 - p4/testdata/p4_16_samples/issue3650.p4 (Failed)
660 - p4/testdata/p4_16_samples/issue3884.p4 (Failed)
681 - p4/testdata/p4_16_samples/issue461-bmv2.p4 (Failed)
705 - p4/testdata/p4_16_samples/issue561-bmv2.p4 (Failed)
741 - p4/testdata/p4_16_samples/issue982.p4 (Failed)
768 - p4/testdata/p4_16_samples/logging-bmv2.p4 (Failed)
773 - p4/testdata/p4_16_samples/m_psa-dpdk-non-zero-arg-default-action-08.p4 (Failed)
800 - p4/testdata/p4_16_samples/newtype2.p4 (Failed)
843 - p4/testdata/p4_16_samples/pna-dpdk-add_on_miss1.p4 (Failed)
855 - p4/testdata/p4_16_samples/pna-dpdk-parser-state-err.p4 (Failed)
869 - p4/testdata/p4_16_samples/pna-elim-hdr-copy-dpdk.p4 (Failed)
899 - p4/testdata/p4_16_samples/pna-mux-dismantle.p4 (Failed)
900 - p4/testdata/p4_16_samples/pna-subparser.p4 (Failed)
901 - p4/testdata/p4_16_samples/pna-too-big-label-name-dpdk.p4 (Failed)
913 - p4/testdata/p4_16_samples/predication_issue.p4 (Failed)
914 - p4/testdata/p4_16_samples/predication_issue_1.p4 (Failed)
918 - p4/testdata/p4_16_samples/proliferation1.p4 (Failed)
928 - p4/testdata/p4_16_samples/psa-basic-counter-bmv2.p4 (Failed)
958 - p4/testdata/p4_16_samples/psa-dpdk-non-zero-arg-default-action-08.p4 (Failed)
989 - p4/testdata/p4_16_samples/psa-e2e-cloning-basic-bmv2.p4 (Failed)
991 - p4/testdata/p4_16_samples/psa-example-counters-bmv2.p4 (Failed)
992 - p4/testdata/p4_16_samples/psa-example-digest-bmv2.p4 (Failed)
1009 - p4/testdata/p4_16_samples/psa-example-dpdk-varbit-bmv2.p4 (Failed)
1034 - p4/testdata/p4_16_samples/psa-i2e-cloning-basic-bmv2.p4 (Failed)
1041 - p4/testdata/p4_16_samples/psa-meter7-bmv2.p4 (Failed)
1042 - p4/testdata/p4_16_samples/psa-multicast-basic-2-bmv2.p4 (Failed)
1043 - p4/testdata/p4_16_samples/psa-multicast-basic-bmv2.p4 (Failed)
1044 - p4/testdata/p4_16_samples/psa-multicast-basic-corrected-bmv2.p4 (Failed)
1045 - p4/testdata/p4_16_samples/psa-parser-error-test-bmv2.p4 (Failed)
1047 - p4/testdata/p4_16_samples/psa-recirculate-no-meta-bmv2.p4 (Failed)
1048 - p4/testdata/p4_16_samples/psa-register-complex-bmv2.p4 (Failed)
1049 - p4/testdata/p4_16_samples/psa-register-read-write-2-bmv2.p4 (Failed)
1050 - p4/testdata/p4_16_samples/psa-register-read-write-bmv2.p4 (Failed)
1055 - p4/testdata/p4_16_samples/psa-resubmit-bmv2.p4 (Failed)
1061 - p4/testdata/p4_16_samples/psa-unicast-or-drop-bmv2.p4 (Failed)
1062 - p4/testdata/p4_16_samples/psa-unicast-or-drop-corrected-bmv2.p4 (Failed)
1071 - p4/testdata/p4_16_samples/redundant_parsers_dangling_unused_parser_decl.p4 (Failed)
1126 - p4/testdata/p4_16_samples/stack-bmv2.p4 (Failed)
1127 - p4/testdata/p4_16_samples/stack-bvec-bmv2.p4 (Failed)
1135 - p4/testdata/p4_16_samples/std_meta_inlining.p4 (Failed)
1153 - p4/testdata/p4_16_samples/subparser-with-header-stack-bmv2.p4 (Failed)
1182 - p4/testdata/p4_16_samples/two-functions.p4 (Failed)
1184 - p4/testdata/p4_16_samples/two_ebpf.p4 (Failed)
1210 - p4/testdata/p4_16_samples/v1model-special-ops-bmv2.p4 (Failed)
1229 - p4/testdata/p4_16_samples/dash/dash-pipeline-pna-dpdk.p4 (Failed)
1230 - p4/testdata/p4_16_samples/dash/dash-pipeline-v1model-bmv2.p4 (Failed)
1231 - p4/testdata/p4_16_samples/fabric_20190420/fabric.p4 (Failed)
1232 - p4/testdata/p4_16_samples/omec/up4.p4 (Failed)
1233 - p4/testdata/p4_16_samples/pins/pins_fabric.p4 (Failed)
1234 - p4/testdata/p4_16_samples/pins/pins_middleblock.p4 (Failed)
1235 - p4/testdata/p4_16_samples/pins/pins_wbb.p4 (Failed)
1250 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test1.p4 (Failed)
1252 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test11.p4 (Failed)
1253 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test12.p4 (Failed)
1255 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test2.p4 (Failed)
1256 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test3.p4 (Failed)
1257 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test4.p4 (Failed)
1258 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test5.p4 (Failed)
1259 - p4/testdata/p4_16_samples/parser-inline/parser-inline-test6.p4 (Failed)
so these changes at least seem to be accomplishing their intended purpose.
If I understand what you are saying, this means that the context representation you are using is incomplete. You need to keep the child number as well.
If I understand what you are saying, this means that the context representation you are using is incomplete. You need to keep the child number as well.
It may be incomplete, but it is still an improvement over the current implementation, so additional improvements can be made separately. And the future improvements will need to affect both loc_t and ProgramPoint, not just loc_t.
It may be incomplete, but it is still an improvement over the current implementation, so additional improvements can be made separately. And the future improvements will need to affect both
loc_tandProgramPoint, not justloc_t.
Maybe we can use some well-known and proven approaches? E.g. some flavour of value numbering?
It may be incomplete, but it is still an improvement over the current implementation, so additional improvements can be made separately. And the future improvements will need to affect both
loc_tandProgramPoint, not justloc_t.Maybe we can use some well-known and proven approaches? E.g. some flavour of value numbering?
That would probably be even better. We can consider doing this in a future PR.
This PR should be ready now. Please see https://github.com/p4lang/p4c/pull/4810 for the implementation of "some flavour of value numbering" suggested by @asl.
@kfcripps Are there updated benchmarking results? Or the ones above are still valid and there were only correctness changes?
@kfcripps Are there updated benchmarking results? Or the ones above are still valid and there were only correctness changes?
@asl
New results on this branch (as of 50c0f9fbe55e969c167afac3d8204b34348163bd):
test/gtestp4c --gtest_filter=P4CParserUnroll.switch_20160512
7799 ms
7774 ms
7799 ms
ctest -R "p14_to_16/testdata/p4_14_samples/switch_20160512/switch.p4"
23.16 s
22.72 s
22.76 s
Re-run benchmarking in less noisy setup, 10 iterations + warmup:
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
test/gtestp4c-main --gtest_filter=P4CParserUnroll.switch_20160512 |
5.113 ± 0.161 | 4.860 | 5.378 | 1.01 ± 0.04 |
test/gtestp4c --gtest_filter=P4CParserUnroll.switch_20160512 |
5.063 ± 0.126 | 4.883 | 5.203 | 1.00 |
I would say no impact
What about #4385?
@fruffy Although the problem described in #4385 was introduced by the same offending PR as the other mentioned issues, it does not necessarily mean that the root cause is the same. This PR does not fix # 4385.
A test case for #4507 seems to be missing here.
See https://github.com/p4lang/p4c/pull/4797#issue-2406024872.. Mihai's PRs masked the root problem, so I do not know of any test cases that pass with this PR but not on main branch. This PR addresses a theoretical problem (described in https://github.com/p4lang/p4c/issues/4548#issuecomment-2169020037) that probably affects real P4 programs, but I do not currently have any real examples that aren't already working on main branch to test. I believe that #4539 actually fixed the crashes of the exact P4 programs pasted in #4500 and #4507 (so if they weren't added in #4359, they should have been added there, not in this PR).
Should we revert #4727?
We can probably revert it, but based on the changes to test outputs in https://github.com/p4lang/p4c/pull/4727/files, there may be other benefits to leaving the added constant folding pass? Also, to be safe, I'd rather not revert any of Mihai's fixes until all related problems with SimplifyDefUse have been fixed (I opened https://github.com/p4lang/p4c/pull/4810 and https://github.com/p4lang/p4c/issues/4811 to address the remaining issues).
See #4797 (comment).. Mihai's PRs masked the root problem, so I do not know of any test cases that pass with this PR but not on
mainbranch. This PR addresses a theoretical problem (described in #4548 (comment)) that probably affects real P4 programs, but I do not currently have any real examples that aren't already working onmainbranch to test. I believe that #4539 actually fixed the crashes of the exact P4 programs pasted in #4500 and #4507 (so if they weren't added in #4359, they should have been added there, not in this PR).
My main issue is that we are closing #4507 but it looks likes we are not adding the test case reported in it. For the other two issues we have added the tests already. We can add it here or in a separate PR.