[P4Smith] Initialize arrays using tuple expressions#5546
Conversation
|
Test failure seems to be some sort of compiler bug related to inlining. I'll take a closer look and open an issue once I've got a smaller reproducer. Edit: Opened #5549 |
|
@kfcripps As far as I can tell, Smith should have already been able to generate programs exhibiting this bug. Because Smith uses a pseudorandom number generator to generate programs, small changes to how the IR is generated (in particular, the number of IR nodes generated -- this header stack initialization change causes Smith to generate new nodes for each array entry) can have a large impact on other parts of the program. Due to these perturbations, one of the seeds tested by the CI now exhibits the bug, which is why we caught it now. The failing program does have a header stack initialization in it (otherwise the program wouldn't have changed), but it wasn't related to the bug so I removed it when minimizing the reproducer. |
|
@eyg1331 Makes sense, thanks for the explanation! |
|
@fruffy Is there currently any mechanism for easily disabling specific seeds in the P4Smith tests? It seems that the changes in this PR are just exposing an existing bug that is not actually caused by this PR. I'm wondering if we actually need to resolve #5549 first or if there is some other way to proceed. |
Yes, that it is correct. There is a mechanism to adjust the likelihood of a particular generated node using the configurations here https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/smith/common/probabilities.h But it is a bit finnicky. Would be nice to override this per command line.
iirc you should be able to add a known bug here https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/smith/scripts/compilation-test.sh#L7 and it continue generating. |
3dae94e to
d556bad
Compare
|
Since #5549 seems somewhat complex to fix, I figured it would make sense to add it as a known bug for the time being. Rerunning the tests after doing so exposed a new bug, which I've reported in #5560 (and also added as a known bug). Once both issues are fixed, I can open another PR to re-enable checking for both bugs. |
Signed-off-by: Enrico Green <Enrico.Green@amd.com>
Signed-off-by: Enrico Green <Enrico.Green@amd.com>
d556bad to
854c7f3
Compare
|
@fruffy Please merge if the changes look ok to you. |
Signed-off-by: Enrico Green <Enrico.Green@amd.com>
fruffy
left a comment
There was a problem hiding this comment.
BTW, this fuzzer was written and ported fairly quickly by grad students (i.e., me) a couple years back. There is likely a lot of improvements that can be made to make it more stable or more effective. Feel free to make more substantial changes too, if needed. That is fine.
When P4Smith's variable declaration generator was written, P4C didn't have an initializer for arrays (then header stacks). Support has since been added for initializing arrays using tuple expressions, and this PR allows P4Smith to generate such code (instead of leaving the arrays uninitialized).