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

Improve PNA system library organization with smaller usable components #4752

Open
wants to merge 16 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
10 changes: 10 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,16 @@ set (OTHER_HEADERS
add_custom_target(update_includes ALL
COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/*.p4 ${P4C_BINARY_DIR}/p4include

COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/_internal
COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/_internal/pna
COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/_internal/pna/v0_5
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/_internal/pna/v0_5/*.p4 ${P4C_BINARY_DIR}/p4include/_internal/pna/v0_5
COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/_internal/pna/v0_7
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/_internal/pna/v0_7/*.p4 ${P4C_BINARY_DIR}/p4include/_internal/pna/v0_7
COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/_internal/pna/dev
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/_internal/pna/dev/*.p4 ${P4C_BINARY_DIR}/p4include/_internal/pna/dev

COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/bmv2
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/bmv2/psa.p4 ${P4C_BINARY_DIR}/p4include/bmv2
COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/dpdk
Expand Down
5 changes: 4 additions & 1 deletion backends/tc/ebpfCodeGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1235,7 +1235,10 @@ const PNAEbpfGenerator *ConvertToEbpfPNA::build(const IR::ToplevelBlock *tlb) {
!d->is<IR::Type_Error>()) {
if (d->srcInfo.isValid()) {
auto sourceFile = d->srcInfo.getSourceFile();
if (sourceFile.endsWith("/pna.p4")) {
// TODO: Similar logic exists in frontends/p4/toP4/toP4.cpp. Consider replacing
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like the parser should already determine whether an object is source from a systems file and add this information to the source information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree this will be a better solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ChrisDodd Do you think this could be done in the P4 parser? How much work would it be?

// the hard-coding here with a better long term solution.
if (sourceFile.endsWith("/pna.p4") ||
sourceFile.find("p4include/_internal") != nullptr) {
// do not generate standard PNA types
continue;
}
Expand Down
1 change: 1 addition & 0 deletions frontends/common/parser_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ limitations under the License.
* machine, the 'p4includePath' would contain the path on the original machine.
*/
const char *p4includePath = CONFIG_PKGDATADIR "/p4include";
const char *p4includeInternalPath = CONFIG_PKGDATADIR "/p4include/_internal";
const char *p4_14includePath = CONFIG_PKGDATADIR "/p4_14include";

const char *ParserOptions::defaultMessage = "Compile a P4 program";
Expand Down
1 change: 1 addition & 0 deletions frontends/common/parser_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ limitations under the License.
// Standard include paths for .p4 header files. The values are determined by
// `configure`.
extern const char *p4includePath;
extern const char *p4includeInternalPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The discussion in #4742 makes me thing we just should have this be a set. Do not know how difficult this is to implement with all the back ends.

extern const char *p4_14includePath;

// Base class for compiler options.
Expand Down
15 changes: 14 additions & 1 deletion frontends/p4/toP4/toP4.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ bool ToP4::isSystemFile(cstring file) {
return false;
}

// Try to guess whether a file is a "system internal" file
bool ToP4::isSystemInternalFile(cstring file) {
if (noIncludes) return false;
if (file.startsWith(p4includeInternalPath) || file.find("p4include/_internal") != nullptr) {
return true;
}
return false;
}

cstring ToP4::ifSystemFile(const IR::Node *node) {
if (!node->srcInfo.isValid()) return nullptr;
auto sourceFile = node->srcInfo.getSourceFile();
Expand Down Expand Up @@ -165,7 +174,11 @@ bool ToP4::preorder(const IR::P4Program *program) {
* non-system header */

if (includesEmitted.find(sourceFile) == includesEmitted.end()) {
if (sourceFile.startsWith(p4includePath)) {
if (isSystemFile(sourceFile)) {
// Skip the include if it's an internal system header.
// TODO: This is a temporary hack. Need to figure out a better solution.
if (isSystemInternalFile(sourceFile)) continue;

const char *p = sourceFile.c_str() + strlen(p4includePath);
if (*p == '/') p++;
if (P4V1::V1Model::instance.file.name == p) {
Expand Down
1 change: 1 addition & 0 deletions frontends/p4/toP4/toP4.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class ToP4 : public Inspector {
listTerminators.pop_back();
}
bool isSystemFile(cstring file);
bool isSystemInternalFile(cstring file);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the definition of a system internal file. Any private header include prefixed with _internal?

Copy link
Member Author

@qobilidop qobilidop Jul 1, 2024

Choose a reason for hiding this comment

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

This is the hacky part. What happened is that in the following loop, program->objects would contain not only #include <pna.p4> that is directly from the source program, but all the transitive <*.p4> libraries, like <_internal/pna/dev/blocks.p4>. This will lead to compiler errors of duplicate definitions on the ToP4 generated code.

for (auto a : program->objects) {

My current solution is to define this special p4include/_internal/ ("system internal") directory, and filter it out in the ToP4 looping. But I don't think this is a proper solution in the long term.

cstring ifSystemFile(const IR::Node *node); // return file containing node if system file
// dump node IR tree up to depth - in the form of a comment
void dump(unsigned depth, const IR::Node *node = nullptr, unsigned adjDepth = 0);
Expand Down
9 changes: 9 additions & 0 deletions p4include/_internal/pna/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# PNA Standard Libraries

In this directory, we decompose different versions of the standard `pna.p4` library (as specified in https://github.com/p4lang/pna) into smaller reusable components. Actual backends supporting PNA might modify some of these components in their target-specific `pna.p4` (e.g. `p4include/dpdk/pna.p4`).

| Version | Release Date | PNA Repo Commit | PNA Spec Link |
| ------- | ------------ | -------------- | ------------- |
| `dev` | N/A | N/A | https://p4.org/p4-spec/docs/pna-working-draft-html-version.html |
| `v0.7` | 2022-12-22 | [9aef684](https://github.com/p4lang/pna/tree/9aef684a8d8e4d1d8ea34b4277bfd329f33e2254) | https://p4.org/p4-spec/docs/PNA-v0.7.html |
| `v0.5` | 2021-05-18 | [e3eb99d](https://github.com/p4lang/pna/tree/e3eb99dc7d59f7a031bc672f8cf6c300f3d166fb) | https://p4.org/p4-spec/docs/PNA-v0.5.0.html |
44 changes: 44 additions & 0 deletions p4include/_internal/pna/dev/all.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/* Copyright 2020-present Intel Corporation

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#ifndef __PNA_P4__
#define __PNA_P4__

#include <core.p4>

/**
* P4-16 declaration of the Portable NIC Architecture
*/

#include <_internal/pna/dev/types_core.p4>
#include <_internal/pna/dev/types_defns.p4>
#include <_internal/pna/dev/types_defns2.p4>
#include <_internal/pna/dev/funcs_int_to_header.p4>
#include <_internal/pna/dev/types_misc.p4>

#include <_internal/pna/dev/extern_hash.p4>
#include <_internal/pna/dev/extern_checksum.p4>
#include <_internal/pna/dev/extern_counter.p4>
#include <_internal/pna/dev/extern_meter.p4>
#include <_internal/pna/dev/extern_register.p4>
#include <_internal/pna/dev/extern_random.p4>
#include <_internal/pna/dev/extern_action.p4>
#include <_internal/pna/dev/extern_digest.p4>

#include <_internal/pna/dev/types_metadata.p4>
#include <_internal/pna/dev/extern_funcs.p4>
#include <_internal/pna/dev/blocks.p4>

#endif // __PNA_P4__
24 changes: 24 additions & 0 deletions p4include/_internal/pna/dev/blocks.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// BEGIN:Programmable_blocks
parser MainParserT<MH, MM>(
packet_in pkt,
out MH main_hdr,
inout MM main_user_meta,
in pna_main_parser_input_metadata_t istd);

control MainControlT<MH, MM>(
inout MH main_hdr,
inout MM main_user_meta,
in pna_main_input_metadata_t istd,
inout pna_main_output_metadata_t ostd);

control MainDeparserT<MH, MM>(
packet_out pkt,
inout MH main_hdr,
in MM main_user_meta,
in pna_main_output_metadata_t ostd);

package PNA_NIC<MH, MM>(
MainParserT<MH, MM> main_parser,
MainControlT<MH, MM> main_control,
MainDeparserT<MH, MM> main_deparser);
// END:Programmable_blocks
1 change: 1 addition & 0 deletions p4include/_internal/pna/dev/extern_action.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include <_internal/pna/v0_7/extern_action.p4>
1 change: 1 addition & 0 deletions p4include/_internal/pna/dev/extern_checksum.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include <_internal/pna/v0_7/extern_checksum.p4>
1 change: 1 addition & 0 deletions p4include/_internal/pna/dev/extern_counter.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include <_internal/pna/v0_7/extern_counter.p4>
1 change: 1 addition & 0 deletions p4include/_internal/pna/dev/extern_digest.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include <_internal/pna/v0_7/extern_digest.p4>
Loading
Loading