Skip to content
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

@command_line annotation #5123

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion backends/bmv2/pna_nic/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ int main(int argc, char *const argv[]) {

if (program == nullptr || ::P4::errorCount() > 0) return 1;
try {
P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));

P4::FrontEnd frontend;
Expand Down
2 changes: 1 addition & 1 deletion backends/bmv2/psa_switch/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ int main(int argc, char *const argv[]) {

if (program == nullptr || ::P4::errorCount() > 0) return 1;
try {
P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));

P4::FrontEnd frontend;
Expand Down
2 changes: 1 addition & 1 deletion backends/bmv2/simple_switch/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ int main(int argc, char *const argv[]) {

if (program == nullptr || ::P4::errorCount() > 0) return 1;
try {
P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));

P4::FrontEnd frontend;
Expand Down
2 changes: 1 addition & 1 deletion backends/dpdk/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ int main(int argc, char *const argv[]) {

if (program == nullptr || ::P4::errorCount() > 0) return 1;
try {
P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));

P4::FrontEnd frontend;
Expand Down
2 changes: 1 addition & 1 deletion backends/ebpf/p4c-ebpf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void compile(EbpfOptions &options) {
program = P4::parseP4File(options);
if (::P4::errorCount() > 0) return;

P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));

P4::FrontEnd frontend;
Expand Down
2 changes: 1 addition & 1 deletion backends/graphs/p4c-graphs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ int main(int argc, char *const argv[]) {
if (program == nullptr || ::P4::errorCount() > 0) return 1;

try {
P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(false);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));

P4::FrontEnd fe;
Expand Down
4 changes: 0 additions & 4 deletions backends/p4test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,6 @@ set (P4_XFAIL_TESTS
# will be no P4Info file.
p4c_add_tests_w_p4runtime("p4" ${P4TEST_DRIVER} "${P4TEST_SUITES}" "${P4_XFAIL_TESTS}" "${P4RUNTIME_EXCLUDE}" "-a '--maxErrorCount 100'")

set (P4TEST_PARSERUNROLL
"${P4C_SOURCE_DIR}/testdata/p4_16_samples/parser-unroll-*.p4")
p4c_add_tests("p4unroll" ${P4TEST_DRIVER} "${P4TEST_PARSERUNROLL}" "" "-a '--maxErrorCount 100 --loopsUnroll'")

set (P4TEST_PARSER_INLINE_TESTS "${P4C_SOURCE_DIR}/testdata/p4_16_samples/parser-inline/*.p4")
p4c_add_tests("p4" ${P4TEST_DRIVER} "${P4TEST_PARSER_INLINE_TESTS}" "" "-a '--maxErrorCount 100 --parser-inline-opt'")

Expand Down
2 changes: 1 addition & 1 deletion backends/p4test/p4test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ int main(int argc, char *const argv[]) {
info.emitInfo("PARSER");

if (program != nullptr && ::P4::errorCount() == 0) {
P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));
info.emitInfo("PASS P4COptionPragmaParser");

Expand Down
2 changes: 1 addition & 1 deletion backends/p4tools/common/compiler/compiler_target.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ const IR::P4Program *CompilerTarget::runParser(const ParserOptions &options) {

const IR::P4Program *CompilerTarget::runFrontend(const CompilerOptions &options,
const IR::P4Program *program) const {
P4COptionPragmaParser optionsPragmaParser;
P4COptionPragmaParser optionsPragmaParser(false);
program->apply(ApplyOptionsPragmas(optionsPragmaParser));

auto frontEnd = mkFrontEnd();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ p4tools_add_xfail_reason(
# Unexpected error in RPC handling
issue3374.p4
control-hs-index-test6.p4
parser-unroll-test1.p4
)

p4tools_add_xfail_reason(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ p4tools_add_xfail_reason(
# terminate called after throwing an instance of 'std::runtime_error'
# Type is not convertible to string
control-hs-index-test3.p4
parser-unroll-test1.p4
# terminate called after throwing an instance of 'std::out_of_range'
control-hs-index-test2.p4
)
Expand Down
2 changes: 1 addition & 1 deletion backends/tc/tc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ int main(int argc, char *const argv[]) {
return 1;
}
try {
P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));
P4::FrontEnd frontend;
frontend.addDebugHook(hook);
Expand Down
3 changes: 3 additions & 0 deletions backends/tofino/bf-p4c/bf-p4c-options.h
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,9 @@ class BFNOptionPragmaParser : public P4::P4COptionPragmaParser {
public:
std::optional<CommandLineOptions> tryToParse(const IR::Annotation *annotation) override;

BFNOptionPragmaParser() : P4::P4COptionPragmaParser(false) {}
// FIXME -- should remove the tofino @command_line stuff and use the base class

private:
std::optional<CommandLineOptions> parseCompilerOption(const IR::Annotation *annotation);
};
Expand Down
16 changes: 16 additions & 0 deletions frontends/common/applyOptionsPragmas.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,22 @@ std::optional<IOptionPragmaParser::CommandLineOptions> P4COptionPragmaParser::tr
const IR::Annotation *annotation) {
auto pragmaName = annotation->name.name;
if (pragmaName == "diagnostic") return parseDiagnostic(annotation);
if (supportCommandLinePragma && pragmaName == "command_line") {
IOptionPragmaParser::CommandLineOptions options;
auto *args = annotation->needsParsing()
? P4ParserDriver::parseExpressionList(annotation->srcInfo,
annotation->getUnparsed())
: &annotation->getExpr();
for (auto *arg : *args) {
if (auto *a = arg->to<IR::StringLiteral>()) {
options.push_back(a->value.c_str());
} else {
// can this happen? annotation parser should require only strings
warning("ignoring non-string %1% in @command_line", a);
}
}
return options;
}
return std::nullopt;
}

Expand Down
4 changes: 4 additions & 0 deletions frontends/common/applyOptionsPragmas.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,13 @@ class ApplyOptionsPragmas : public Inspector {
* they're unable to parse a pragma.
*/
class P4COptionPragmaParser : public IOptionPragmaParser {
bool supportCommandLinePragma;

public:
std::optional<CommandLineOptions> tryToParse(const IR::Annotation *annotation) override;

explicit P4COptionPragmaParser(bool sCLP) : supportCommandLinePragma(sCLP) {}

private:
std::optional<CommandLineOptions> parseDiagnostic(const IR::Annotation *annotation);
};
Expand Down
3 changes: 3 additions & 0 deletions frontends/p4/parseAnnotations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ ParseAnnotations::HandlerMap ParseAnnotations::standardHandlers() {

// @match has an expression argument
PARSE(IR::Annotation::matchAnnotation, Expression),

// @command_line to add to the command line
PARSE_STRING_LITERAL_LIST("command_line"_cs),
};
}

Expand Down
2 changes: 1 addition & 1 deletion test/gtest/helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ namespace P4::Test {
return std::nullopt;
}

P4::P4COptionPragmaParser optionsPragmaParser;
P4::P4COptionPragmaParser optionsPragmaParser(true);
program->apply(P4::ApplyOptionsPragmas(optionsPragmaParser));
if (::P4::errorCount() > 0) {
std::cerr << "Encountered " << ::P4::errorCount()
Expand Down
4 changes: 3 additions & 1 deletion testdata/p4_16_samples/parser-unroll-issue3537-1.p4
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#include <core.p4>
#include <v1model.p4>

@command_line("--loopsUnroll")

struct H { }

struct M { }
Expand Down Expand Up @@ -68,4 +70,4 @@ control ComputeChecksumI(inout H hdr, inout M meta) {


V1Switch(ParserI(), VerifyChecksumI(), IngressI(), EgressI(),
ComputeChecksumI(), DeparserI()) main;
ComputeChecksumI(), DeparserI()) main;
1 change: 1 addition & 0 deletions testdata/p4_16_samples/parser-unroll-issue3537.p4
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <core.p4>
#include <v1model.p4>
@command_line("--loopsUnroll")

header h1_t {}
header h2_t {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <core.p4>
#include <dpdk/psa.p4>
@command_line("--loopsUnroll")

typedef bit<48> eth_addr_t;

Expand Down
1 change: 1 addition & 0 deletions testdata/p4_16_samples/parser-unroll-t1-cond.p4
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// examples for simple IP/tcp.
#include <core.p4>
#include <v1model.p4>
@command_line("--loopsUnroll")

typedef bit<48> MacAddress;
typedef bit<32> ip4Addr_t;
Expand Down
3 changes: 2 additions & 1 deletion testdata/p4_16_samples/parser-unroll-test1.p4
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "v1model.p4"
@command_line("--loopsUnroll")

const bit<16> TYPE_IPV4 = 0x800;
const bit<16> TYPE_SRCROUTING = 0x1234;
Expand Down Expand Up @@ -102,4 +103,4 @@ control computeChecksum(inout headers hdr, inout metadata meta) {
apply {}
}
V1Switch(MyParser(), verifyChecksum(), mau(), mau(), computeChecksum(),
deparse()) main;
deparse()) main;
1 change: 1 addition & 0 deletions testdata/p4_16_samples/parser-unroll-test10.p4
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <core.p4>
#include <v1model.p4>
@command_line("--loopsUnroll")

header test_h {
bit<8> field;
Expand Down
3 changes: 2 additions & 1 deletion testdata/p4_16_samples/parser-unroll-test2.p4
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "v1model.p4"
@command_line("--loopsUnroll")

const bit<16> TYPE_IPV4 = 0x800;
const bit<16> TYPE_SRCROUTING = 0x1234;
Expand Down Expand Up @@ -99,4 +100,4 @@ control computeChecksum(inout headers hdr, inout metadata meta) {
apply {}
}
V1Switch(MyParser(), verifyChecksum(), mau(), mau(), computeChecksum(),
deparse()) main;
deparse()) main;
3 changes: 2 additions & 1 deletion testdata/p4_16_samples/parser-unroll-test3.p4
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "v1model.p4"
@command_line("--loopsUnroll")

const bit<16> TYPE_IPV4 = 0x800;
const bit<16> TYPE_SRCROUTING = 0x1234;
Expand Down Expand Up @@ -114,4 +115,4 @@ control computeChecksum(inout headers hdr, inout metadata meta) {
apply {}
}
V1Switch(MyParser(), verifyChecksum(), mau(), mau(), computeChecksum(),
deparse()) main;
deparse()) main;
3 changes: 2 additions & 1 deletion testdata/p4_16_samples/parser-unroll-test4.p4
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "v1model.p4"
@command_line("--loopsUnroll")

const bit<16> TYPE_IPV4 = 0x800;
const bit<16> TYPE_SRCROUTING = 0x1234;
Expand Down Expand Up @@ -107,4 +108,4 @@ control computeChecksum(inout headers hdr, inout metadata meta) {
apply {}
}
V1Switch(MyParser(), verifyChecksum(), mau(), mau(), computeChecksum(),
deparse()) main;
deparse()) main;
3 changes: 2 additions & 1 deletion testdata/p4_16_samples/parser-unroll-test5.p4
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "v1model.p4"
@command_line("--loopsUnroll")

const bit<16> TYPE_IPV4 = 0x800;
const bit<16> TYPE_SRCROUTING = 0x1234;
Expand Down Expand Up @@ -109,4 +110,4 @@ control computeChecksum(inout headers hdr, inout metadata meta) {
apply {}
}
V1Switch(MyParser(), verifyChecksum(), mau(), mau(), computeChecksum(),
deparse()) main;
deparse()) main;
1 change: 1 addition & 0 deletions testdata/p4_16_samples/parser-unroll-test6.p4
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* -*- Mode:C; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
#include <v1model.p4>
@command_line("--loopsUnroll")

header test_header_t {
bit<8> value;
Expand Down
1 change: 1 addition & 0 deletions testdata/p4_16_samples/parser-unroll-test7.p4
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <core.p4>
#include <v1model.p4>
@command_line("--loopsUnroll")

header S {
bit<8> t;
Expand Down
1 change: 1 addition & 0 deletions testdata/p4_16_samples/parser-unroll-test8.p4
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <core.p4>
#define V1MODEL_VERSION 20180101
#include <v1model.p4>
@command_line("--loopsUnroll")

header h_index1 {
bit<8> index;
Expand Down
1 change: 1 addition & 0 deletions testdata/p4_16_samples/parser-unroll-test9.p4
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <core.p4>
#define V1MODEL_VERSION 20180101
#include <v1model.p4>
@command_line("--loopsUnroll")

header h_stack {
bit<8> i1;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <core.p4>
@command_line("--loopsUnroll")

parser p(packet_in packet, out bit<8> f, out bit<4> g) {
state start {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <core.p4>
@command_line("--loopsUnroll")

struct S {
bit<8> f;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#define V1MODEL_VERSION 20180101
#include <v1model.p4>

struct H {
@command_line("--loopsUnroll") struct H {
}

struct M {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#define V1MODEL_VERSION 20180101
#include <v1model.p4>

struct H {
@command_line("--loopsUnroll") struct H {
}

struct M {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#define V1MODEL_VERSION 20180101
#include <v1model.p4>

struct H {
@command_line("--loopsUnroll") struct H {
}

struct M {
Expand All @@ -11,9 +11,6 @@ struct M {
parser ParserI(packet_in packet, out H hdr, inout M meta, inout standard_metadata_t smeta) {
@name("ParserI.tmp_0") bit<16> tmp_0;
@name("ParserI.tmp_2") bit<16> tmp_2;
state start {
transition s1;
}
state s1 {
packet.advance(32w16);
tmp_0 = packet.lookahead<bit<16>>();
Expand All @@ -34,6 +31,9 @@ parser ParserI(packet_in packet, out H hdr, inout M meta, inout standard_metadat
state s4 {
transition accept;
}
state start {
transition s1;
}
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does start get moved here in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something the ParsersUroll pass does . I checked these outputs against the deleted ones in parser-unroll (which was the outputs for the tests with the explicit --loopsUnroll argument in ctest) and they match.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the midend outputs changing now because:

  • Previously, these outputs were the result of running the corresponding tests without --loopsUnroll. The outputs in p4_16_samples_outputs/parser-unroll/ were the result of running the same set of tests with --loopsUnroll.
  • Now, we are only running each of these tests one time - only with --loopsUnroll. So the set of outputs corresponding to no --loopsUnroll were deleted and replaced by outputs of the same set of tests corresponding to having --loopsUnroll.

Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

Copy link
Contributor

Choose a reason for hiding this comment

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

So now we have less test coverage (for cases which don't run the ParsersUnroll midend pass). I don't know the history of the parser unroll tests enough to know if this is ok or not. Either

  1. they were added with the only intention being to exercise the ParsersUnroll pass, so it's fine that we're no longer exercising the paths that don't go through the ParsersUnroll pass, or
  2. they were added with the intention to exercise both paths, in which case we still need to exercise the paths not through ParsersUnroll. In this case we may need to add keep two versions of each of these tests - one with @command_line(--loopsUnroll), and one without.

tagging @jafingerhut @fruffy @vlstill in case they know the history better than me

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless someone chimes in with knowledge of the history from memory, the git commit log for who added those tests is probably the best way to find out who knows the history here.

Copy link
Collaborator

@fruffy fruffy Feb 21, 2025

Choose a reason for hiding this comment

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

they were added with the only intention being to exercise the ParsersUnroll pass, so it's fine that we're no longer exercising the paths that don't go through the ParsersUnroll pass, or

This was the case. If we do not want to lose some coverage, we can rename the files slightly (append the -bmv2 suffix) such that they are picked up by the BMv2 tests. These do not use loopsunrolling iirc.

Edit:
We probably do not need to change anything because P4Testgen also exercises these tests. P4Testgen uses the loopsunrolling pass but compiles the program with the p4c-bm2-ss compiler, which does not use unrolling.

Edit2:
Turns out the BMv2 back end supports the loops unroll option and it is enabled with this PR, so we should consider disabling it if we want to preserve test coverage.

}

control IngressI(inout H hdr, inout M meta, inout standard_metadata_t smeta) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#define V1MODEL_VERSION 20180101
#include <v1model.p4>

struct H {
@command_line("--loopsUnroll") struct H {
}

struct M {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
parser-unroll-issue3537-1.p4(43): [--Wwarn=unused] warning: Control Aux is not used; removing
parser-unroll-issue3537-1.p4(45): [--Wwarn=unused] warning: Control Aux is not used; removing
control Aux(inout M meta) {
^^^
[--Wwarn=invalid] warning: Parser cycle can't be unrolled, because ParserUnroll can't detect the number of loop iterations:
Parser ParserI state chain: start, s1, s2, s2
[--Wwarn=invalid] warning: Parser cycle can't be unrolled, because ParserUnroll can't detect the number of loop iterations:
Parser ParserI state chain: start, s1, s2, s1
[--Wwarn=invalid] warning: Parser cycle can't be unrolled, because ParserUnroll can't detect the number of loop iterations:
Parser ParserI state chain: start, s1, s1
Loading
Loading