8000 [bmv2]: allow BoolLiteral in ternary table entry by grg · Pull Request #5199 · p4lang/p4c · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[bmv2]: allow BoolLiteral in ternary table entry #5199

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 27, 2025
Merged

[bmv2]: allow BoolLiteral in ternary table entry #5199

merged 2 commits into from
Mar 27, 2025

Conversation

grg
Copy link
Contributor
@grg grg commented Mar 26, 2025 •
edited
Loading

Allow use of BoolLiteral (true/false) in a table const entry in ternary field.

Example (from test case):

    table test_table {
        key = {
            hdr.ethernet.isValid() : ternary;
        }
        actions = { ... }
        const entries = {
            (true) : DummyAction();
        }
    }

@fruffy / @jafingerhut : Please let me know if this is in violation of the P4 spec. AFAIK, it should be fine.

@grg
bmv2: allow BoolLiteral in ternary table entry
89745cc 
Allow use of BoolLiteral (true/false) in a table const entry in ternary
field.

Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
@grg grg requested review from jafingerhut and fruffy March 26, 2025 20:52
@grg grg marked this pull request as ready for review March 26, 2025 20:52
@jafingerhut
Copy link
Contributor

Allow use of BoolLiteral (true/false) in a table const entry in ternary field.

Example (from test case):

    table test_table {
        key = {
            hdr.ethernet.isValid() : ternary;
        }
        actions = { ... }
        const entries = {
            (true) : DummyAction();
        }
    }

@fruffy / @jafingerhut : Please let me know if this is in violation of the P4 spec. AFAIK, it should be fine.

I don't recall anything that this would violate in the language spec.

If you wanted a const entry that was wildcard on the Boolean field, it isn't clear to me how one would specify that, but the lack of such a capability need not block this change.

It appears that for P4Info file generation, table match keys with type boolean are represented as bit<1>, which makes sense. I don't recall off-hand when there might have been a change to enable that behavior -- probably years ago.

@grg
Copy link
Contributor Author
grg commented Mar 26, 2025

If you wanted a const entry that was wildcard on the Boolean field, it isn't clear to me how one would specify that, but the lack of such a capability need not block this change.

This is already supported by placing an underscore (_) in place of the true.

@grg
Copy link
Contributor Author
grg commented Mar 26, 2025

Unfortunately a couple of the other backends don't support this:

The following tests FAILED:
	 15 - dpdk/testdata/p4_16_samples/psa-bool-ternary-const-entry-bmv2.p4 (Failed) dpdk
	1177 - p4/testdata/p4_16_samples/psa-bool-ternary-const-entry-bmv2.p4 (Failed) p4
Errors while running CTest

I had hoped that by including -bmv2 in the name that the other backends wouldn't pick it up. Guess I'll have to fix those backends or at least adjust their xfails 🙁

@jafingerhut
Copy link
Contributor

Unfortunately a couple of the other backends don't support this:

The following tests FAILED:
	 15 - dpdk/testdata/p4_16_samples/psa-bool-ternary-const-entry-bmv2.p4 (Failed) dpdk
	1177 - p4/testdata/p4_16_samples/psa-bool-ternary-const-entry-bmv2.p4 (Failed) p4
Errors while running CTest

I had hoped that by including -bmv2 in the name that the other backends wouldn't pick it up. Guess I'll have to fix those backends or at least adjust their xfails 🙁

I am not sure, but one of the failures looked like it was because it could not find a .txtpb expected output file in the testdata/p4_16_samples_outputs directory to compare against. Maybe try generating and adding those files to the PR?

@grg
add missing test outputs
fdbeb95 
Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
@grg
Copy link
Contributor Author
grg commented Mar 26, 2025

I had hoped that by including -bmv2 in the name that the other backends wouldn't pick it up. Guess I'll have to fix those backends or at least adjust their xfails 🙁

I am not sure, but one of the failures looked like it was because it could not find a .txtpb expected output file in the testdata/p4_16_samples_outputs directory to compare against. Maybe try generating and adding those files to the PR?

Yes, upon further investigation, it was just the missing test outputs. So an easy fix 🙂

@fruffy fruffy changed the title bmv2: allow BoolLiteral in ternary table entry [bmv2]: allow BoolLiteral in ternary table entry Mar 27, 2025
@fruffy fruffy added the bmv2 Topics related to BMv2 or v1model label Mar 27, 2025
jafingerhut
jafingerhut approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor
@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

LGTM

@grg grg added this pull request to the merge queue Mar 27, 2025
Merged via the queue into p4lang:main with commit de37731 Mar 27, 2025
21 of 22 checks passed
@grg grg deleted the gleng/bmv2-bool-in-table-const-entry branch March 27, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jafingerhut jafingerhut jafingerhut approved these changes

@fruffy fruffy Awaiting requested review from fruffy

Assignees
No one assigned
Labels
bmv2 Topics related to BMv2 or v1model
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grg @jafingerhut @fruffy
0